Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Investigate moment map commented test #2936

Closed
wants to merge 2 commits into from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jul 2, 2024

This PR addresses the following unreleased code (hence no change log needed):

  • Removing the need to create temporary spectrum just to convert unit for moment map, hence removing the need to "Centralize pixel scale factor information".
    • Update: The whole changes from Jesse in moment map are now reverted. Moment map will stay in surface brightness.
  • Fixing compatibility with unreleased specutils (hence fixing devdeps).
  • Fixing a possible bug in flux_conversion function where equivalencies math is flipped for cases len(values) != 2 and len(values) == 2. This is accomplished by having both logic go through the same math (there is really no good reason to have separate math for those cases anyway).

Out of scope:

  • Fallback value for PIXAR_SR when it is not found. Personally, I think any fallback is dangerous unless you can be sure you are able to look up the actual value from somewhere (e.g., handbooks, databases, websites) for any given telescope/instrument combo. Otherwise, better to force user input or just disallow it from happening.

xref #2910 #2873

🐱

@pllim pllim added trivial Only needs one approval instead of two no-changelog-entry-needed changelog bot directive labels Jul 2, 2024
@pllim pllim added this to the 3.11 milestone Jul 2, 2024
@github-actions github-actions bot added cubeviz plugin Label for plugins common to multiple configurations labels Jul 2, 2024
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.78%. Comparing base (e65237f) to head (2a120e2).
Report is 161 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2936      +/-   ##
==========================================
- Coverage   88.79%   88.78%   -0.02%     
==========================================
  Files         111      111              
  Lines       17236    17233       -3     
==========================================
- Hits        15305    15300       -5     
- Misses       1931     1933       +2     

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

@pllim pllim modified the milestones: 3.11, 4.0 Jul 2, 2024
@pllim pllim removed the trivial Only needs one approval instead of two label Jul 2, 2024
@pllim pllim marked this pull request as ready for review July 2, 2024 21:17
@pllim pllim requested a review from astrofrog July 2, 2024 21:17
@pllim pllim modified the milestone: 4.0 Jul 2, 2024
@javerbukh
Copy link
Contributor

Do moment maps work for you in this PR? I see an error with all default values in the CubevizExample notebook
UnitConversionError: 'MJy / sr' (surface brightness) and 'MJy' (spectral flux density) are not convertible

@pllim
Copy link
Contributor Author

pllim commented Jul 3, 2024

Do moment maps work for you in this PR?

Is this using that thingy that is not enabled by default?

@javerbukh
Copy link
Contributor

No, but the auto-collapsed spectrum (using the sum function) is in flux units which means the default units are in flux, while moment map is using surface brightness units.

@pllim
Copy link
Contributor Author

pllim commented Jul 3, 2024

Didn't @kecnry ask us to just leave it broken until Gilbert's thingy is in or something?

@kecnry
Copy link
Member

kecnry commented Jul 3, 2024

It probably makes most sense to merge the changes to display units first, rebase this on top of that, and then ensure that it correctly handles SB<>SB conversions correctly without unnecessary code for SB<>flux which won't be supported.

@pllim
Copy link
Contributor Author

pllim commented Jul 3, 2024

Why is the CI green? What are we not testing?

@pllim
Copy link
Contributor Author

pllim commented Jul 3, 2024

Re: #2936 (comment)

Okay, so we need to keep this code for now, yes? In that case, it can still be simplified and there is still a bug to be fixed.

@pllim
Copy link
Contributor Author

pllim commented Jul 3, 2024

I cleaned it up (again). What about now?

@kecnry
Copy link
Member

kecnry commented Jul 5, 2024

We will be keeping SB<>SB unit conversion but will no longer need SB<>flux (the comments seem to suggest the opposite).

@pllim
Copy link
Contributor Author

pllim commented Jul 5, 2024

Comments aside (which I can update), the code itself looks okay to proceed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lots of duplicated logic in flux_conversion. It was hurting my head, so I ended up just re-writing it to be simpler.

@pllim
Copy link
Contributor Author

pllim commented Jul 10, 2024

OK I think I am done here until I hear more.

to not require spectrum, so nothing to centralize now.

Fix devdeps

Simplify flux_conversion logic
and other minor fixes. Add flux conversion tests.

Clarify which one should be reverted
in the future
because moment map always in SB
@pllim
Copy link
Contributor Author

pllim commented Jul 19, 2024

The code here is now part of #2940

@pllim pllim closed this Jul 19, 2024
@pllim pllim deleted the but-which-one branch July 19, 2024 20:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cubeviz no-changelog-entry-needed changelog bot directive plugin Label for plugins common to multiple configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants