fornax-navo / fornax-demo-notebooks

Demo notebooks for the Fornax project
https://fornax-navo.github.io/fornax-demo-notebooks/
BSD 3-Clause "New" or "Revised" License
7 stars 19 forks source link

ML_AGNzoo suggestions #268

Closed troyraen closed 2 months ago

troyraen commented 2 months ago

This is a partial review of ML_AGNzoo.md.

Suggested changes

logging and warnings

Silencing all warnings hides important information about things that may be going wrong. I see that there are a large number of log messages being generated in the notebook that are not warnings but are more like info messages and lower. I think this is happening because sompy sets logging levels to NOTSET and DEBUG, which is very noisy. I think a good solution is to reset the logging level to the default WARNING, which is what I've done here. This silences most of the log messages without hiding the warnings.

With the logging level set to WARNING, several cells are now showing warnings. For example: RuntimeWarning: invalid value encountered in divide. I recommend checking each of these since they indicate there may be a problem with the math.

Consolidate code

There is quite a bit of duplicated code in this notebook, mostly for plotting. I picked one cell (which plots ZTF data) and consolidated the code to show an example of how it can be done. It would be nice if the rest of the code were similarly consolidated. Even if we don't plan to do much more work on this notebook, I think the futures users would appreciate it. It will make the notebook easier to read and much easier to change (which I think we should expect that users will do) without introducing bugs.

xoubish commented 2 months ago

Thanks Troy. All looks good to me. One reason for the repetitions in plotting was if people wanted to use parts of the code standalone, they could without the need to refer too much to previous cells. I also added gdown which skips the google virus scanner you mentioned.

troyraen commented 2 months ago

The code within each cell could be consolidated independently from the other cells.