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

Handling larger range for BUNIT conversions #700

Merged

Conversation

e-koch
Copy link
Contributor

@e-koch e-koch commented Feb 4, 2021

This is to address #683. There are a number of fairly common unit transitions that we've been missing. We also have some gaps for different objects that can be addressed here (e.g., 2D Projection or Slice).

SpectralCube

  • Jy/beam <-> Jy/pix
  • Jy/sr <-> Jy/beam
  • K <-> Jy/sr
  • K <-> Jy/pix

VaryingResolutionSpectralCube

  • K <-> Jy/beam
  • Jy/beam <-> Jy/pix
  • Jy/sr <-> Jy/beam
  • K <-> Jy/sr
  • K <-> Jy/pix

Projection/Slice

  • K <-> Jy/beam
  • Jy/beam <-> Jy/pix
  • Jy/sr <-> Jy/beam
  • K <-> Jy/sr
  • K <-> Jy/pix
  • K km/s <-> Jy/beam (km/s) (integrated intensity)
  • [ ] km/s <-> m/s (centroid/line width) (Unsure why I added this. It's a basic transform that already works.)

OneDSpectrum

VaryingResolutionOneDSpectrum

  • K <-> Jy/beam

@codecov-io
Copy link

codecov-io commented Mar 11, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@71fd8e2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #700   +/-   ##
=========================================
  Coverage          ?   77.66%           
=========================================
  Files             ?       24           
  Lines             ?     5579           
  Branches          ?        0           
=========================================
  Hits              ?     4333           
  Misses            ?     1246           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71fd8e2...4891302. Read the comment docs.

@e-koch e-koch changed the title [WIP] Handling larger range for BUNIT conversions Handling larger range for BUNIT conversions Mar 16, 2021
@e-koch
Copy link
Contributor Author

e-koch commented Apr 29, 2021

All brightness unit conversions should be handled and tested now.

Integrated units like K km/s aren't working yet.

@e-koch e-koch force-pushed the jybeam_beam_change_unit_conversion branch from 42786d8 to a27f2f4 Compare April 29, 2021 21:04
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2021

Codecov Report

Merging #700 (1a53cb7) into master (8bb9a37) will decrease coverage by 3.68%.
The diff coverage is 86.36%.

❗ Current head 1a53cb7 differs from pull request most recent head a27f2f4. Consider uploading reports for the commit a27f2f4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   80.30%   76.62%   -3.69%     
==========================================
  Files          24       24              
  Lines        5551     5475      -76     
==========================================
- Hits         4458     4195     -263     
- Misses       1093     1280     +187     
Impacted Files Coverage Δ
spectral_cube/cube_utils.py 80.71% <84.93%> (+0.55%) ⬆️
spectral_cube/lower_dimensional_structures.py 81.41% <87.50%> (-2.56%) ⬇️
spectral_cube/base_class.py 85.17% <100.00%> (-2.61%) ⬇️
spectral_cube/spectral_cube.py 75.91% <100.00%> (-8.05%) ⬇️
spectral_cube/visualization-tools.py 0.00% <0.00%> (-12.25%) ⬇️
spectral_cube/ytcube.py 18.81% <0.00%> (-10.90%) ⬇️
spectral_cube/dask_spectral_cube.py 84.60% <0.00%> (-4.59%) ⬇️
spectral_cube/masks.py 79.41% <0.00%> (-4.03%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bb9a37...a27f2f4. Read the comment docs.

@e-koch e-koch requested review from keflavich and astrofrog May 6, 2021 13:53
@e-koch
Copy link
Contributor Author

e-koch commented May 6, 2021

@astrofrog @keflavich -- This is ready to review for BUNIT conversions. Composite unit conversions like Jy/beam km/s -> K km/s are not working, but I'm in favour of splitting that into another PR.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Only nitpicks.

Per our discussion, don't merge until #709 is solved

def pixels_per_beam(self):
pixels_per_beam = [(beam.sr /
(astropy.wcs.utils.proj_plane_pixel_area(self.wcs) *
u.deg**2)).to(u.one).value for beam in self.beams]
Copy link
Contributor

Choose a reason for hiding this comment

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

WHOA, I didn't know u.one is u.dimensionless! neat!

spectral_cube/cube_utils.py Outdated Show resolved Hide resolved
spectral_cube/cube_utils.py Outdated Show resolved Hide resolved
spectral_cube/cube_utils.py Outdated Show resolved Hide resolved
@e-koch e-koch force-pushed the jybeam_beam_change_unit_conversion branch from b658365 to 30de78e Compare May 6, 2021 21:28
@e-koch
Copy link
Contributor Author

e-koch commented May 6, 2021

Tests passing now that #709 is merged. The remaining failures for the windows build are from a yt issue.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Can we add at least one specific by-hand correctness check?

@keflavich keflavich merged commit e8342f7 into radio-astro-tools:master May 13, 2021
@e-koch e-koch deleted the jybeam_beam_change_unit_conversion branch May 13, 2021 17:28
@e-koch e-koch mentioned this pull request May 19, 2022
7 tasks
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.

4 participants