Closed apizzuto closed 3 years ago
I just realized that I forgot to tag the PR about documentation https://github.com/SNEWS2/snewpy/pull/112
Another small note: in the SNEWS2.0_whitepaper_rates
notebook, there are some hardcoded paths in the "Execute SNOwGLoBES" section
Thanks for being so thorough in your checks!
For the notebooks that relate to a specific model, would it perhaps be better to include them in models/<MODEL_NAME>/
? That way, they would also be downloaded automatically whenever someone uses $ python -c 'import snewpy; snewpy.get_models()'
and be present right next to the model files in the same directory.
I’ll try to do this and fix the other issues you mention in the coming days; I am unfortunately very busy currently with a collaboration meeting and a detector monitoring shift.
I think that would totally make sense for a place to put them!
No rush on getting these things done. I think you have all of my comments from the review now, so I'll happily standby and give the official green light after these last few things get fixed
Regarding Flavor_XForm.ipynb
and FlavorTransformation.ipynb
: @jpkneller, can you take a look at those? As I recall, FlavorTransformation.ipynb
contains the plots you made for the ApJ paper. Maybe we should keep that one, merge in any bits from the Flavor_XForm
notebook that you think are worth keeping and then delete the Flavor_XForm
one? I agree with @apizzuto that it’s confusing to have two different notebooks that demo the same aspect of SNEWPY.
Sure thing.
Flavor_XForm
and FlavorTransformation
are fixed in #116. In #117, I’ve already moved the model notebooks to their respective directories and fixed the issues in the Sukhbold_2015
and Fornax_2019
notebooks. I’ll try to get to the other ones in the next few days.
I’ve just added commits that fix the paths in snowglobes_models.ipynb
and delete SNEWS2.0_whitepaper_rates.ipynb
, since it was an outdated example; the updated one with more detectors and the necessary fixes is included as a separate example script.
… and I’ve renamed SNEWPY_examples.ipynb
to SNOwGLoBES_usage.ipynb
and added a README file describing the remaining notebooks. Plus some additional cleanup in the last few commits. I think that covers all your comments @apizzuto!?
You bet! Thank you @JostMigenda. Feel free to merge the draft PR and that's my last comment on the software!
Hey SNEWPY team! This is part of https://github.com/openjournals/joss-reviews/issues/3772.
These are a summary of some of the minor issues I ran into when working through the (truly impressive) notebooks that you all put together. First, I have a request for documentation: could you add a brief README to the directory where the notebooks live, briefly explaining what each one is for? It was a bit daunting at first when I saw there were so many and wasn't sure where to start. It's up to you, but it might be worth separating those that demonstrate core functionality (eg.
SNEWPY_examples.ipynb
) from those just highlighting a single model by putting all of the model ones in a subdirectory.As for specific issues:
SNEWPY_examples.ipynb
: Would you be able to add a little bit of text to the beginning of the notebook (or throughout) that describes what's happening in this notebook? The inline comments are helpful, but some overview at the beginning would help provide contextsnowglobes_models.ipynb
: all of the models with "Type Ia" hadFileNotFoundErrors
because it was a space instead of an underscore (sn5, sn6) in the filenameSNEWS2.0_whitepaper_rates
: I think you need afrom snewpy.models import Bollig2016
, otherwise in the Generate Fluence Files section I get aNameError: name 'Bollig2016' is not defined
FlavorTransformations.ipynb
: I'm guessing this is an artifact of a weird PYTHONPATH error from whoever wrote this notebook, but it looks like the imports don't work here because they are trying to import frompython.snewpy
instead ofsnewpy
Flavor_XForm
and all of the model notebooks: No issue here, I just really enjoyed working through them :)Fornax_2019.ipynb
:ImportError
because some of the later plots requirehealpy
. Is this functionality crucial for SNEWPY and shouldhealpy
be a dependency? If not, can you just add a disclaimer to the top of the notebook about how this notebook will only work if the user hashealpy
?Sukhbold_2015.ipynb
:fig = plot_luminosity(model)
gives an error:AttributeError: 'Sukhbold_2015' object has no attribute 'fitsfile'
from the title line in the call toax.plot
. Is it supposed to bemodel.filename
?Flavor_XForm.ipynb
andFlavorTransformations.ipynb
were