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

Migrate to tardis composition #188

Merged

Conversation

jvshields
Copy link
Contributor

@jvshields jvshields commented Jun 6, 2024

Switch over stardis to using the shared tardis Composition class.

There's also some small cleanups. Fixed a lot of inconsistent spacing in the broadening test file, and got rid of hydrogen mass as an input to some broadening functions in favor of just passing in a constant.

I've also included a fix for the broken benchmarks (unpinning the ASV version) but I don't think that will propagate into the github workflow for this PR because I believe it executes the workflow specified in the main branch.

@jvshields jvshields changed the title Improve chemical mass fraction functionality Migrate to tardis composition Jun 11, 2024
@jvshields jvshields added the benchmarks Trigger benchmarks to run on this pr label Jun 11, 2024
@tardis-bot
Copy link
Contributor

tardis-bot commented Jun 11, 2024

*beep* *bop*
Hi human,
I ran benchmarks as you asked comparing main (74ce810) and the latest commit (f1fa39c).
Here are the logs produced by ASV.
Results can also be downloaded as artifacts here.
Significantly changed benchmarks:

· Benchmark list file .asv/results/benchmarks.json missing!
Use `asv run --bench just-discover` to regenerate benchmarks.json

All benchmarks:

· Benchmark list file .asv/results/benchmarks.json missing!
Use `asv run --bench just-discover` to regenerate benchmarks.json

@@ -86,4 +84,14 @@ def parse_config_to_model(config_fname):
continuum_interaction_species=[],
)

# if (
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with this big comment block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The big comment block is to allow for rescaled mass fractions (which was the original purpose of this PR. After discussion, we decided that I should use tardis composition and then add that functionality to it, rather than expanding stardis composition functionality. I wanted to keep this code around and ready to uncomment (or at least be a starting point) for after I implement the rescaling into tardis composition. This also applies to the config expansion I did, which is currently unused.

benchmarks/run_stardis.py Outdated Show resolved Hide resolved
benchmarks/run_stardis.py Outdated Show resolved Hide resolved
stardis/conftest.py Outdated Show resolved Hide resolved
stardis/io/model/marcs.py Outdated Show resolved Hide resolved
@jvshields jvshields merged commit c440059 into tardis-sn:main Jun 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Trigger benchmarks to run on this pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants