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

Use builtin Astropy model unit and bounding box support #238

Closed
wants to merge 1 commit into from

Conversation

lpsinger
Copy link
Contributor

Define the input_units, return_units, input_units_equivalencies, and bounding_box properties for all models. Use Astropy models' built-in unit conversion support.

All models now require inputs with valid units (wavelength, wavenumber, or frequency). Dimensionless inputs are no longer automatically converted to wavenumber.

Fixes #237.

@lpsinger
Copy link
Contributor Author

Note that there is one unit test that is failing due to astropy/astropy#16906.

@karllark
Copy link
Owner

Can you say how the valid x range in checked with these changes? Just can't see how this is done and figured I'd just ask.

@karllark
Copy link
Owner

And the unit test failing will need to be addressed.

@lpsinger
Copy link
Contributor Author

Can you say how the valid x range in checked with these changes? Just can't see how this is done and figured I'd just ask.

I overrode the ``astropy.modeling.Model.prepare_inputsmethod inBaseExtModel` because it turns out that Astropy itself doesn't actually do bounds checking based on the bounding box except in rare cases like Tabular1D.

@lpsinger
Copy link
Contributor Author

And the unit test failing will need to be addressed.

Yup. Waiting to hear back from upstream about astropy/astropy#16906.

@karllark
Copy link
Owner

Can you say how the valid x range in checked with these changes? Just can't see how this is done and figured I'd just ask.

I overrode the ``astropy.modeling.Model.prepare_inputsmethod inBaseExtModel` because it turns out that Astropy itself doesn't actually do bounds checking based on the bounding box except in rare cases like Tabular1D.

That is clever! Neat.

Define the `input_units`, `return_units`, `input_units_equivalencies`,
and `bounding_box` properties for all models. Use Astropy models' built-in
unit conversion support.

All models now require inputs with valid units (wavelength, wavenumber, or
frequency). Dimensionless inputs are no longer automatically converted to
wavenumber.

Fixes karllark#237.
@lpsinger
Copy link
Contributor Author

lpsinger commented Sep 5, 2024

I misunderstood what the bounding box is for. It's the "limits over which the model is significant", i.e., far from zero. However, the x_range property of the dust extinction models is really describing the limits over which the model is known or defined. These are not the same thing.

I'm closing this PR. Instead, here's one that just moves the input preparation to the base class and leverages the builtin model input units handling: #240.

See https://docs.astropy.org/en/stable/modeling/models.html#efficient-model-rendering-with-bounding-boxes:

All Model subclasses have a bounding_box attribute that can be used to set the limits over which the model is significant. This greatly improves the efficiency of evaluation when the input range is much larger than the characteristic width of the model itself.

@lpsinger lpsinger closed this Sep 5, 2024
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