BlackBody and units support within modeling plugin #953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #953 +/- ##
==========================================
+ Coverage 69.09% 70.39% +1.29%
==========================================
Files 69 73 +4
Lines 4935 5256 +321
==========================================
+ Hits 3410 3700 +290
- Misses 1525 1556 +31
Continue to review full report at Codecov.
|
kecnry
requested review from
duytnguyendtn,
ibusko,
javerbukh,
ojustino,
pllim and
rosteen
as code owners
kecnry
force-pushed
the
modeling-bb-and-units-support
branch
2 times, most recently
from
613acc3 to
5ae04b9
Compare
* currently uses custom "fork" of BlackBody model with support for flux units. Will likely want to update this to the eventual support for flux units in astropy
* to update to new changes to models with units but without having to duplicate too much code logic
* but remove deprecation tests
kecnry
force-pushed
the
modeling-bb-and-units-support
branch
from
5ae04b9 to
cc2a84d
Compare
* BlackBody now passes unitless scale with output_units assigned from the y-axis of the spectrum * BlackBody.output_units now supports passing units as strings (as it needs to be serializable within jdaviz) * the unitless scale is estimated from the maximum value of the spectrum converted into the native blackbody units (to handle cases where the y-unit contains 1e-17, etc) * automatic limits of (0, None) are placed on the scale to try to prevent the optimization from getting stuck at 0 if overshooting, but can still be somewhat sensitive to the initial guess. * fixes bug (likely introduced earlier in this PR) where overridden values from initializers were not being set correctly in the plugin. This does currently require looping over parameters twice and might be able to be simplified in the future.
kecnry
force-pushed
the
modeling-bb-and-units-support
branch
from
cc2a84d to
3dd2ef5
Compare
|
Ok, this passes all of my tests. I approve to merge! |
pllim
reviewed
Nov 23, 2021
| """ | ||
| Initialization that is specific to the BlackBody model. | ||
|
|
||
| Notes |
There was a problem hiding this comment.
I am not going to comment on the Sphinx stuff here because a hidden class will not make it to public API doc. FYI.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: P. L. Lim <[email protected]>
kecnry
force-pushed
the
modeling-bb-and-units-support
branch
from
ce18253 to
ad97141
Compare
kecnry
force-pushed
the
modeling-bb-and-units-support
branch
from
c85fb35 to
67d8eb5
Compare
pllim
reviewed
Nov 29, 2021
rosteen
approved these changes
Nov 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds support for a BlackBody model in the modeling plugin (which in turn requires proper unit support for models in both jdaviz and specutils). Note that this PR is dependent on astropy/specutils#891 (now merged, but not yet released). Logic for Polynomial1D has been refactored to avoid duplicating code for the new support of units. Since astropy's BlackBody model does not (yet) support flux units, this uses a custom implementation until a proper solution is adopted in astropy.
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.CHANGES.rst?