Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model registry cleanup #230

Merged
merged 14 commits into from
Nov 23, 2022
Merged

Model registry cleanup #230

merged 14 commits into from
Nov 23, 2022

Conversation

JostMigenda
Copy link
Member

@JostMigenda JostMigenda commented Nov 11, 2022

This fixes #224. Two things I want to highlight in particular:

  • It implements this suggestion by @Sheshuk to move the parameter checking into the appropriate base class
  • It removes the Model.param_combinations property in favour of Model.get_param_combinations() (which already existed anyway). In addition to saving some boilerplate code, this is also more extensible. For example, I could imagine adding the ability to give additional arguments like get_param_combinations(eos='shen') to narrow the list down.

During the review, please note that thus far, I’ve only updated Nakazato_2013.ipynb with the new model initialisation. Let me know if that looks reasonable; if so, I will update

  • the notebooks for all other models and
  • the Getting Started page of the documentation

before we can merge this PR.

This makes model loaders simpler, since they don’t have to deal with the FileHandle.
Also clears outputs from notebook, to make diffs clearer in the future
Also removes `param_combinations` property. Instead, call
`get_param_combinations` directly. This saves boilerplate
code and is more extensible.
previous code changed the type since [val] was no longer a Quantity
@JostMigenda JostMigenda added this to the v1.3 milestone Nov 11, 2022
@github-actions
Copy link

github-actions bot commented Nov 14, 2022

Result of Benchmark Tests

Benchmark Min Max Mean
python/snewpy/test/test_05_snowglobes.py::test_simulation_chain_benchmark 4.32 4.45 4.38 +- 0.05

Copy link
Contributor

@sgriswol sgriswol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! The comments I have should be very easy to address. The most important one being, I think snewpy.models._init_model should be public-facing. Is there a reason it is named with a leading underscore?

python/snewpy/models/__init__.py Outdated Show resolved Hide resolved
models/Nakazato_2013/Nakazato_2013.ipynb Outdated Show resolved Hide resolved
Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
@JostMigenda
Copy link
Member Author

snewpy.models._init_model should be public-facing.

Eventually, yes. But for now, I think it’s not yet ready—see #225. For this PR, I just wanted to move that out of the util.py file so I could delete that as part of the cleanup.

Copy link
Contributor

@sgriswol sgriswol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this all together @JostMigenda! I have a few comments, most of which are cosmetic and should be easy to address.

  • The citation links OConnor_2013.ipynb , Zha_2021.ipynb, and Warren_2020.ipynb are missing.
  • The output of code cell 3 in Nakazato_2013.ipynb is rather hard to read.
  • The population of valid eos and progenitor_mass parameters in the docstring of snewpy.models.ccsn.OConnor_2013 isn't happening because python/snewpy/models/ccsn.py:415 is commented.
  • In many of the example notebooks I received a warning over importing from snewpy.models rather than snewpy.models.ccsn. I have added suggestions to change most notebooks, but I was unable to make a suggestion for Fornax_2019.ipynb or OConnor_2013.ipynb

I will push a commit shortly to address these comments, aside from the suggestions I was able to make as part of the review (apologies for the mixed formats!).

models/Bollig_2016/Bollig_2016.ipynb Outdated Show resolved Hide resolved
models/Kuroda_2020/Kuroda_2020.ipynb Outdated Show resolved Hide resolved
models/OConnor_2015/OConnor_2015.ipynb Outdated Show resolved Hide resolved
models/Sukhbold_2015/Sukhbold_2015.ipynb Outdated Show resolved Hide resolved
models/Tamborra_2014/Tamborra_2014.ipynb Outdated Show resolved Hide resolved
models/Walk_2019/Walk_2019.ipynb Outdated Show resolved Hide resolved
models/Warren_2020/Warren_2020.ipynb Outdated Show resolved Hide resolved
models/Warren_2020/Warren_2020.ipynb Outdated Show resolved Hide resolved
python/snewpy/models/__init__.py Outdated Show resolved Hide resolved
models/Zha_2021/Zha_2021.ipynb Outdated Show resolved Hide resolved
Co-authored-by: Spencer Griswold <sgriswold@icecube.wisc.edu>
Copy link
Contributor

@sgriswol sgriswol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@JostMigenda JostMigenda merged commit e28ec7a into main Nov 23, 2022
@JostMigenda JostMigenda deleted the JostMigenda/ModelRegistryCleanup branch November 23, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup of model registry
2 participants