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

Implementing preSN registry models #296

Merged
merged 18 commits into from
Apr 8, 2024

Conversation

Sheshuk
Copy link
Contributor

@Sheshuk Sheshuk commented Dec 12, 2023

Closes #243
Closes #290

  • Fix model tests which were not using absolute file paths (test_02_models.py)
  • Moved GarchingArchiveModel from base to the loaders module.
  • Created new LocalFileLoader class for checking if the requested file exists
  • And RegistryFileLoader to download the requested file from the _model_downloader

Todo:

  • Implement RegistryModels for preSN
  • Add tests for preSN models
  • Add preSN models to the documentation

@Sheshuk Sheshuk marked this pull request as ready for review December 13, 2023 14:46
@Sheshuk Sheshuk self-assigned this Feb 10, 2024
@JostMigenda JostMigenda self-requested a review March 28, 2024 13:52
@JostMigenda
Copy link
Member

I’ve finished looking through the code-level changes; they all look good to me. Thanks a lot, @Sheshuk!

Before I accept and merge this PR, I’d like to try some usage examples. I noticed that the demo notebooks (e.g. https://github.com/SNEWS2/snewpy-models-presn/blob/master/models/Patton_2017/Patton.ipynb) still use the old (filename-based) initalisation, which doesn’t work with this PR. Have you started updating those already, or should I go ahead, update them as I try them out and make a PR on the snewpy-models-presn repo?

@Sheshuk
Copy link
Contributor Author

Sheshuk commented Apr 3, 2024

Thanks @JostMigenda! I will update these examples, they're rather old in general.

JostMigenda and others added 3 commits April 7, 2024 14:00
Also fixes a bug where a scalar value for E (e.g. 10*u.MeV) would not work, and a length-1 array (e.g. [10]*u.MeV) was required
Copy link
Member

@JostMigenda JostMigenda left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!

@Sheshuk Sheshuk merged commit 4cc8ceb into main Apr 8, 2024
10 checks passed
@Sheshuk Sheshuk deleted the Sheshuk/add_preSN_models_to_registry branch April 8, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants