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

update PhysicalModel.get_band_fluxes() in physical_model.py #200

Closed
wants to merge 1 commit into from

Conversation

mi-dai
Copy link
Contributor

@mi-dai mi-dai commented Dec 19, 2024

An error occured when filter_name in opsim was not included in PassbandGroup when calculating the fluxes using PhysicalModel.get_band_fluxes(). I added a check.

@mi-dai mi-dai requested review from hombit and OliviaLynn December 19, 2024 20:49
Comment on lines +283 to +284
filter_mask = filters == filter_name
band_fluxes[:, filter_mask] = np.nan
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd fail instead - it would help catching typos in field names

Suggested change
filter_mask = filters == filter_name
band_fluxes[:, filter_mask] = np.nan
raise ValueError(f"Filter '{filter_name}' is not found in the passbands object, available filters are {list(passband_or_group.passbands)}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, then we'll have to modify opsim to make sure it doesn't output non-overlapping filters, or we'll now get this error instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest we don't raise an error for now.

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 the state of this PR? Is there consensus on whether to fail? If so, can we re-review and merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we bypass this issue by forcing the opsim to have same set of filters as passband_group. I'm closing it for now.

Copy link

Before [5ab23f3] After [3df7107] Ratio Benchmark (Parameter)
5.83±0.9ms 5.05±1ms ~0.87 benchmarks.TimeSuite.time_chained_evaluate
2.31±0.8ms 1.52±0.8ms ~0.66 benchmarks.TimeSuite.time_fnu_to_flam
1.38±0.05ms 1.47±0.04ms 1.06 benchmarks.TimeSuite.time_apply_passbands
5.04±0.2ms 5.03±0.08ms 1.00 benchmarks.TimeSuite.time_evaluate_salt3_passbands
28.1±0.3μs 28.1±0.3μs 1.00 benchmarks.TimeSuite.time_make_new_salt3_model
128±2μs 128±2μs 1.00 benchmarks.TimeSuite.time_sample_x0_from_distmod
1.40±0s 1.39±0.01s 0.99 benchmarks.TimeSuite.time_make_x1_from_hostmass
8.94±0.1ms 8.77±0.09ms 0.98 benchmarks.TimeSuite.time_load_passbands
16.7±0.3μs 16.3±0.5μs 0.98 benchmarks.TimeSuite.time_sample_x1_from_hostmass
11.0±0.4ms 10.4±0.5ms 0.94 benchmarks.TimeSuite.time_evaluate_salt3_model

Click here to view all benchmarks.

@mi-dai mi-dai closed this Jan 7, 2025
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.

3 participants