Fornax 2022 updates - Githubissues

SNEWS2 / snewpy

A Python package for working with supernova neutrinos

https://snewpy.readthedocs.io

BSD 3-Clause "New" or "Revised" License

24 stars 17 forks source link

Fornax 2022 updates #283

Closed sybenzvi closed 8 months ago

sybenzvi commented 9 months ago

New model class and loader for the 100 axisymmetric (2D) simulations produced by the Princeton group using the Fornax code. Includes a notebook that tests I/O for the models.

The model data live in the fornax-2022-data branch of snewpy-models-ccsn.

JostMigenda commented 9 months ago

Thanks for working on this!

Initial comments:

[x] Since this is a newly added model, I think we don’t have to worry about backward compatibility and can skip the whole “deprecated filename argument” song and dance we’ve had to do for other models.
[x] Before merging, we shouldn’t forget to add tests.
[x] Have you reproduced some basic plots from the original paper to verify the accuracy of the implementation? If so, it’d be nice to post them here in the thread.

I’m currently at NNN, but will try to take a closer look at this once I’m back.

sybenzvi commented 9 months ago

Thanks @JostMigenda , I am pretty swamped the next two days but it shouldn't be hard to complete the tests and checks you requested.

sybenzvi commented 9 months ago

@JostMigenda , in reply to your suggestions:

[x] Remove the filename argument and deprecation warnings: done.
[x] Add tests: done.
[x] Reproduce plots and check results: unfortunately the two reference papers do not have plots that can be compared to the outputs, but I added a plotting notebook in the fornax-2022-data branch of the snewpy-models-ccsn repo and everything looks reasonable.

For example, here is luminosity vs. time for 5 models: fornax_lum_test

And here are corresponding energy spectra: fornax_spec_test

sybenzvi commented 8 months ago

After the comments by Jost implemented, my only comment is that all other models have a jupyer notebook with an example on how to initialise and use the model, which is missing for Fornax_2022 now. However, as we are going to avoid having the model data in snewpy/models (where the notebooks are) as of now, I am not sure it is really needed.

@mcolomermolla , I actually put the demo notebook in the new snewpy-models-ccsn project that we're using to store the model files.

We can and probably should have all the notebooks moved out of that project and the corresponding deprecated models folder in this project, but I'd like to keep that issue separate from this PR.

mcolomermolla commented 8 months ago

After the comments by Jost implemented, my only comment is that all other models have a jupyer notebook with an example on how to initialise and use the model, which is missing for Fornax_2022 now. However, as we are going to avoid having the model data in snewpy/models (where the notebooks are) as of now, I am not sure it is really needed.

@mcolomermolla , I actually put the demo notebook in the new snewpy-models-ccsn project that we're using to store the model files.

We can and probably should have all the notebooks moved out of that project and the corresponding deprecated models folder in this project, but I'd like to keep that issue separate from this PR.

I agree, we can move all data files and the corresponding notebooks there. Then, I see no stop point to merge this PR!