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

Missing directions/masses in Walk and Tamborra models #231

Merged
merged 22 commits into from
Jun 2, 2023

Conversation

mcolomermolla
Copy link
Collaborator

@mcolomermolla mcolomermolla commented Nov 16, 2022

This PR will close issue #78.

  • Incorporate data for Walk_2018 and modify README with new parameters documentation
  • Incorporate data for Walk_2019 and modify README with new parameters documentation
  • Include new parameters in the models class in models/ccsn.py
  • Modify the notebooks with new model initialisation for Walk models
  • Incorporate missing masses and directions for Tamborra_2014

For Tambora_2014, I am in touch with Irene, who told me that they did not save the theta and phi used in the three directions of the paper, so for now I can't provide all directions for all masses.

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.

Thanks for adding these models! A couple things I’ve noticed:

models/Walk_2018/README.md Show resolved Hide resolved
models/Walk_2019/README.md Show resolved Hide resolved
python/snewpy/models/ccsn.py Outdated Show resolved Hide resolved
python/snewpy/models/ccsn.py Outdated Show resolved Hide resolved
mcolomermolla and others added 2 commits November 16, 2022 18:21
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
@JostMigenda
Copy link
Member

Regarding tests: In test_01_registry.py and test_02_models.py, the model parameters for the Walk models need to be updated as well. Also, when adding these new files, we need to bump the release_version in model_files.yml for the Walk_2018 and Walk_2019 classes to 1.3 to ensure the model downloader finds the right files.

Slight problem: Until we’ve tagged v1.3, the model downloader won’t be able to download the files, so we can’t test that. (@sgriswol, am I missing something here?)

@sgriswol
Copy link
Contributor

You're right @JostMigenda, the model downloader attempts to grab models files using a URL that includes a specific version tag (1.2 currently). So until these new model are available under a specific tag, they can't be downloaded. The downloader also needs the correct version to be set in snewpy.models.model_files.yml.

Slight problem: Until we’ve tagged v1.3, the model downloader won’t be able to download the files, so we can’t test that. (@sgriswol, am I missing something here?)

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.

I took a quick look and noticed that the new parameter options for Walk_2018 and Walk_2019 were missing from the __new__ methods in a few places. Let me know if I can help get these integrated!

python/snewpy/models/ccsn.py Outdated Show resolved Hide resolved
python/snewpy/models/ccsn.py Outdated Show resolved Hide resolved
python/snewpy/models/ccsn.py Outdated Show resolved Hide resolved
python/snewpy/models/ccsn.py Outdated Show resolved Hide resolved
mcolomermolla and others added 4 commits November 22, 2022 14:56
Include new parameters in check - Walk2018

Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Add missing arguments for new parameters - Walk2018

Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Add new parameters in check - Walk2019

Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Add missing new parameters - Walk2019

Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
@mcolomermolla
Copy link
Collaborator Author

I took a quick look and noticed that the new parameter options for Walk_2018 and Walk_2019 were missing from the __new__ methods in a few places. Let me know if I can help get these integrated!

Thanks @sgriswol for your review. Your suggested changes have been added.

mcolomermolla and others added 6 commits May 15, 2023 15:08
`(1)` is an int in unnecessary brackets; an added comma makes it a tuple
assert appropriate Errors for non-existant parameter combinations
fix missing "dir" in filename template
@JostMigenda
Copy link
Member

I added some small fixes for the tests. The remaining test failures are due to the download issue mentioned above; that’s unfortunately impossible to fix within the scope of this PR.

However, when I manually copy the newly added model files to the cache directory (the model_path defined in snewpy/__init__.py) on my computer and then run the tests locally, they succeed.

@JostMigenda JostMigenda self-requested a review June 2, 2023 15:37
@JostMigenda JostMigenda force-pushed the mcolomer/missing_param_tamborra_walk branch from 033ad4a to 4f73372 Compare June 2, 2023 15:55
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.

Hi @mcolomermolla! I’ve reverted your last commit (updating the model notebooks), because that was already done in another PR (after this one was created), so it created a conflict with the main branch.

Otherwise this looks good; just one more update to the README, then it’s ready to merge

models/Tamborra_2014/README.md Outdated Show resolved Hide resolved
@JostMigenda JostMigenda merged commit cd675a6 into main Jun 2, 2023
4 of 8 checks passed
@JostMigenda JostMigenda deleted the mcolomer/missing_param_tamborra_walk branch June 2, 2023 16:21
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.

None yet

3 participants