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 model fit result units after toggling flux/surface brightness #3113

Merged
merged 6 commits into from
Jul 29, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Jul 23, 2024

This PR makes the plotted model fit results update properly when toggling between flux and surface brightness. It also removes one use of aperture_area_along_spectral that was missed in #3088 that was leading to the converted model fit being incorrect.

@github-actions github-actions bot added cubeviz plugin Label for plugins common to multiple configurations labels Jul 23, 2024
@rosteen rosteen added this to the 4.0 milestone Jul 23, 2024
Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Works great, thanks!

assert np.allclose(value[0], 4.800000041882413e-08)
assert np.allclose(value[0], 8e-11)
Copy link
Member

Choose a reason for hiding this comment

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

wait, is this change expected? Is this because of the change to remove aperture_area_along_spectral? (cc @bmorris3)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe so, it looked like this value comes from code that would have been affected by that change.

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.90%. Comparing base (143cd8d) to head (147ce2e).
Report is 2 commits behind head on main.

Files Patch % Lines
...igs/default/plugins/model_fitting/model_fitting.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3113      +/-   ##
==========================================
+ Coverage   88.78%   88.90%   +0.11%     
==========================================
  Files         111      111              
  Lines       17372    17378       +6     
==========================================
+ Hits        15424    15450      +26     
+ Misses       1948     1928      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@cshanahan1 cshanahan1 left a comment

Choose a reason for hiding this comment

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

I noticed one small potential bug when testing this out - if you create several different components with different units in a mix of flux / SB, and then combine those to fit the model, you get a message in red text in the plugin saying that units are incompatible and to remove the offending components. this message lingers even after you remove them from the equation editor

also, in theory couldn't you convert a model component added when the display units are in SB back to flux or vise versa so model components created when the viewer was in flux or when the viewer was in SB can work together?

@rosteen
Copy link
Collaborator Author

rosteen commented Jul 29, 2024

also, in theory couldn't you convert a model component added when the display units are in SB back to flux or vise versa so model components created when the viewer was in flux or when the viewer was in SB can work together?

In theory yes, but that's outside the scope of this PR.

I noticed one small potential bug when testing this out - if you create several different components with different units in a mix of flux / SB, and then combine those to fit the model, you get a message in red text in the plugin saying that units are incompatible and to remove the offending components. this message lingers even after you remove them from the equation editor

I'll look into this.

@rosteen
Copy link
Collaborator Author

rosteen commented Jul 29, 2024

@cshanahan1 The warning appears to be working properly for me, it disappears if you either delete the incompatible component from the plugin, or just delete it in the equation editor (video below is only the first case but it works for both).

Screen.Recording.2024-07-29.at.7.26.17.AM.mov

@rosteen rosteen merged commit dc2186e into spacetelescope:main Jul 29, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants