search

delve-team / delve

PyTorch model training and layer saturation monitor
https://delve-docs.readthedocs.io
MIT License
79 stars 13 forks source link

[JOSS review] API #58

Closed dionhaefner closed 2 years ago

dionhaefner commented 2 years ago

I wonder if CheckLayerSat is really the best name for your main tracker object. The imperative sounds more like a function name to me, and Sat is so overloaded that it's not obvious what it stands for. I would probably use something like SaturationTracker or so.

But I understand that changing names in the public API can be a pain, so if you insist to keep it that's fine with me.

(This is a part of the ongoing review at openjournals/joss-reviews#3992)

MLRichter commented 2 years ago

Hi, I agree, and I think changing it is a good Idea, since the bugfix is trivial for the API is remaining unchanged otherwise. This is currently implemented in PR as part of a code cleanup effort and waiting for approval. https://github.com/delve-team/delve/pull/63