search

logix-project / logix

AI Logging for Interpretability and Explainability🔬
Apache License 2.0
86 stars 6 forks source link

Delete args from Analog.__call__ #52

Closed hwijeen closed 10 months ago

hwijeen commented 10 months ago

https://github.com/sangkeun00/analog/pull/49#discussion_r1424119537

log, hessian, save are the three arguments that Analog keeps track of. It has to be changed at each epoch, not at each batch. This PR deletes the Analog's function to alter the value at each batch. These values will be set properly at every epoch by AnalogScheduler.

sangkeun00 commented 10 months ago

I have a mixed opinion about this. In case we AnaLogScheduler, we won't pass anything in __call__, so nothing will be updated. I just wonder whether there are some users who want to play around with various things, and having this option would make this exploration easier. I agree that setting configs in __call__ won't be needed in most cases though. Do you think having this option would just rather cause confusion?

hwijeen commented 10 months ago

I see your point. In general, I think having flexibility is a good idea. But can we think of a specific use case where users (or even us) would want to toggle these options by batch? If not, I think we gain more from simplifying (passing only batch-related args to analog.__call__) the code.

I stress simplicity here as we keep different sets of variables (log, save, hessian), (synchronize, save) and I think it would be worthwhile for each set to have distinct concept in order to make code more approachable.

sangkeun00 commented 10 months ago

Sorry for the confusion. I think there indeed are not many cases where users want to try out different configs for different batches. However, I do think users may want to set this argument in __call__ instead of doing this in update. One of the feedback I got from people yesterday was that this update is somewhat unintuitive from the user perspective. It's probably more explicit to do this when you call analog in the context manager.

I stress simplicity here as we keep different sets of variables (log, save, hessian), (synchronize, save) and I think it would be worthwhile for each set to have distinct concept in order to make code more approachable.

I also couldn't fully understand this statement. Regardless of our __call__ design, those variables are saved/tracked in AnaLog anyway.

Overall, I do agree that this is somewhat redundant when we already have update, and we can deprecate this in the future. However, at the moment, I think it's better to give users both options, see what they prefer, and reflect this in our future design. I think this contributes little to the readability of our code.

hwijeen commented 10 months ago

It sounds reasonable. Thanks for the opinion!