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

Add a helper function for assembling a snia graph #210

Closed
wants to merge 7 commits into from

Conversation

jeremykubica
Copy link
Contributor

Instead of having users always manually recreate all the nodes needed for each graph, we can prebuild sections of the graph. snia_x0_x1_from_host() creates and links the nodes used by snia models.

@jeremykubica jeremykubica requested a review from mi-dai January 2, 2025 19:17
Copy link

github-actions bot commented Jan 2, 2025

Before [1f3a6d9] After [9c7f1eb] Ratio Benchmark (Parameter)
6.52±0.8ms 5.87±0.9ms ~0.90 benchmarks.TimeSuite.time_chained_evaluate
2.63±0.9ms 2.03±0.9ms ~0.77 benchmarks.TimeSuite.time_fnu_to_flam
1.53±0.09ms 1.54±0.04ms 1.01 benchmarks.TimeSuite.time_apply_passbands
5.11±0.07ms 5.17±0.06ms 1.01 benchmarks.TimeSuite.time_evaluate_salt3_passbands
11.0±0.5ms 11.0±0.1ms 1.00 benchmarks.TimeSuite.time_evaluate_salt3_model
8.72±0.03ms 8.72±0.1ms 1.00 benchmarks.TimeSuite.time_load_passbands
28.7±0.5μs 28.8±0.3μs 1.00 benchmarks.TimeSuite.time_make_new_salt3_model
1.38±0.01s 1.37±0.01s 0.99 benchmarks.TimeSuite.time_make_x1_from_hostmass
128±0.9μs 127±1μs 0.99 benchmarks.TimeSuite.time_sample_x0_from_distmod
16.6±0.3μs 16.4±0.4μs 0.99 benchmarks.TimeSuite.time_sample_x1_from_hostmass

Click here to view all benchmarks.

Copy link
Contributor

@mi-dai mi-dai 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 good to me. My concern is now we lose the flexibility to define x1_func in other ways since it's already pre-defined in this snia_x0_x1_from_host function.

@jeremykubica
Copy link
Contributor Author

It is a trade off. The question for you is whether it is worth having this as a default implementation or whether the user should specify it themselves in the simulation (as in your demo notebook). I think either way will be fine.

@mi-dai
Copy link
Contributor

mi-dai commented Jan 3, 2025

I think it's better to keep the way in the demo notebook, because it shows clearly how each function is defined. If user would like to keep all pre-defined functions we should wrap everything into one function instead of only wrapping x0 and x1.

@jeremykubica
Copy link
Contributor Author

Sounds good. Closing this PR.

@jeremykubica jeremykubica deleted the helper_functions branch January 3, 2025 15:50
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.

2 participants