Skip to content

Allow more unit types for pixel_scale - #10123

Merged
mhvk merged 1 commit into
astropy:masterfrom
Jerry-Ma:generalize_pixel_scale
Apr 16, 2020
Merged

Allow more unit types for pixel_scale#10123
mhvk merged 1 commit into
astropy:masterfrom
Jerry-Ma:generalize_pixel_scale

Conversation

@Jerry-Ma

@Jerry-Ma Jerry-Ma commented Apr 7, 2020

Copy link
Copy Markdown
Contributor

Description

This pull request is to address the need of pixel equivalencies on images where pixels are not related to distances on the sky.

This proposed implementation does not assume an equivalency of the pixscale unit to u.rad/u.pix (or u.pix/u.rad), instead, it constructs the correct scaling relation by decomposing the unit and inspect the dimensionality of u.pix in the unit of pixscale.

Fixes #10110

@astropy-bot astropy-bot Bot added the units label Apr 7, 2020
@pllim
pllim requested review from eteq and mhvk April 7, 2020 21:58
@pllim pllim added this to the v4.1 milestone Apr 7, 2020
pllim
pllim reviewed Apr 7, 2020
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Needs a change log and additional text in https://astropy.readthedocs.io/en/latest/units/equivalencies.html#pixel-and-plate-scale-equivalencies .

I've tagged others to review this in detail. At a glance, I think the composite units should be created first before being applied to the values, hence the suggested parentheses.

Comment thread astropy/units/tests/test_equivalencies.py Resolved
Comment thread astropy/units/tests/test_equivalencies.py Resolved
@nden

nden commented Apr 8, 2020

Copy link
Copy Markdown
Contributor

@Jerry-Ma Could you add a test that it serializes to asdf?
Adding an example to the list here will do it. Thanks.

astropy/astropy/io/misc/asdf/tags/unit/tests/test_equivalency.py

Line 14 in 6191daa

def get_equivalencies():

@Jerry-Ma

Jerry-Ma commented Apr 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@nden Just added the Asdf round trip test.

However, there is a can of worm that I opted not to open by choosing the test case, because I think the issue is bigger than the scope of this pull request.

In short, the Asdf round trip test would fail on a test case like this: pixel_scale(100. * u.pix / u.imperial.inch).

The reason is that Asdf serialize the argument in an alternated form for some units. In this case, the scaled form of u.imperial.inch is used. This results in the serialized file as follows:

#ASDF 1.0.0
#ASDF_STANDARD 1.4.0
%YAML 1.1
%TAG ! tag:stsci.edu:asdf/
--- !core/asdf-1.1.0
asdf_library: !core/software-1.0.0 {author: Space Telescope Science Institute, homepage: 'http://github.com/spacetelescope/asdf',
  name: asdf, version: 2.5.2}
history:
  extensions:
  - !core/extension_metadata-1.0.0
    extension_class: asdf.extension.BuiltinExtension
    software: {name: asdf, version: 2.5.2}
  - !core/extension_metadata-1.0.0
    extension_class: astropy.io.misc.asdf.extension.AstropyAsdfExtension
    software: {name: astropy, version: 4.1.dev917+g8267d16fa.d20200407}
  - !core/extension_metadata-1.0.0
    extension_class: astropy.io.misc.asdf.extension.AstropyExtension
    software: {name: astropy, version: 4.1.dev917+g8267d16fa.d20200407}
eq: !<tag:astropy.org:astropy/units/equivalency-1.0.0>
- kwargs_names: [pixscale]
  kwargs_values:
  - !unit/quantity-1.1.0 {unit: !unit/unit-1.0.0 0.39370079cm-1 pixel, value: 100.0}
  name: pixel_scale
inch: !unit/unit-1.0.0 2.54cm
per_inch: !unit/unit-1.0.0 0.39370079cm-1
...

Then, it is easy to see that when this file is read and the new pixel_scale is constructed, the kwargs will be of different units. In this particular case, we have:

>>> dpi_rt.kwargs[0]  # the round tripped value
{'pixscale': <Quantity 100. 0.393701 pix / cm>}
>>> dpi.kwargs[0]  # the input value
{'pixscale': <Quantity 100. pix / inch>}

This will fail the assertion of equality because the __eq__ of Equivalency class not setup to handle this situation (it only compares kwargs by doing a.kwargs == b.kwargs). However, these two parameters are indeed the same quantity, as the assertion assert_quantity_allclose(dpi.kwargs[0]['pixscale'], dpi_rt.kwargs[0]['pixscale']) passes.

mhvk
mhvk reviewed Apr 8, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Jerry-Ma - this is really nice, thanks! I have one comment about the implementation that is really more about what was there than what you added, but we might as well change it... I give a specific suggestion inline.

Comment thread astropy/units/equivalencies.py Resolved
Comment thread astropy/units/equivalencies.py Resolved
Comment thread astropy/units/tests/test_equivalencies.py Resolved
Comment thread docs/units/equivalencies.rst Resolved
mhvk
mhvk reviewed Apr 8, 2020
View reviewed changes
Comment thread docs/units/equivalencies.rst Resolved
mhvk
mhvk reviewed Apr 8, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks all OK modulo a small typo. While getting rid of that, would you mind rebasing & squashing so it is just 1 commit? Thanks!

@Jerry-Ma
Jerry-Ma force-pushed the generalize_pixel_scale branch from dd62813 to ce29781 Compare April 8, 2020 20:04
@Jerry-Ma

Jerry-Ma commented Apr 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@mhvk Did the squash and rebase, forgot to ping you.

mhvk
mhvk approved these changes Apr 16, 2020
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Jerry-Ma - thanks for the ping. This looks all OK now, so merging! Thanks for the nice improvement!

@mhvk
mhvk merged commit d91f444 into astropy:master Apr 16, 2020
@pllim pllim mentioned this pull request Apr 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@mhvk mhvk mhvk approved these changes
@eteq eteq Awaiting requested review from eteq

Assignees

No one assigned

Labels

Enhancement units

Projects

None yet

Milestone

v4.1

Development

Successfully merging this pull request may close these issues.

Add a new unit equivalency to handle pixel scale specified with length unit.

4 participants

@Jerry-Ma @nden @pllim @mhvk