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

Rewrite np.percentile section to handle units for AstroPy 3, 4. #116

Merged
merged 3 commits into from Feb 11, 2020

Conversation

wmwv
Copy link
Contributor

@wmwv wmwv commented Feb 11, 2020

No description provided.

@wmwv wmwv requested a review from timj February 11, 2020 15:37
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks great. Nice clarification as well.


# In AstroPy 3, numpy.percentitle of an array with quantities
# (incorrectly) returns a unitless number
# In AstroPy 4, numpy.percentitle of an array with Quantities
Copy link
Member

Choose a reason for hiding this comment

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

I think it's Astropy4+numpy >=1.17.

# In AstroPy 3, numpy.percentitle of an array with quantities
# (incorrectly) returns a unitless number
# In AstroPy 4, numpy.percentitle of an array with Quantities
# correct returns a number with the same units as the input array.
Copy link
Member

Choose a reason for hiding this comment

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

correct -> correctly

# 1. Extract the array in marcsec and convert to a value.
# 2. Calculate the numpy.percentitle of this unitless array
# 3. Multiply that result by u.marcsec to get the right units.
rmsDistMas = amx.extras['rmsDistMas'].quantity.to(u.marcsec).value,
Copy link
Member

Choose a reason for hiding this comment

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

There's a comma at the end of this line that should not be there.

@timj
Copy link
Member

timj commented Feb 11, 2020

I also needed to apply this patch:

diff --git a/python/lsst/validate/drp/calcsrd/pa2.py b/python/lsst/validate/drp/calcsrd/pa2.py
index 6dd6c0a..8b98c47 100644
--- a/python/lsst/validate/drp/calcsrd/pa2.py
+++ b/python/lsst/validate/drp/calcsrd/pa2.py
@@ -59,5 +59,5 @@ def measurePA2(metric, pa1, pf1_thresh):
     magDiffs = pa1.extras['magDiff'].quantity[0, :]
 
     pf1Percentile = 100.*u.percent - pf1_thresh
-    return Measurement(metric, np.percentile(np.abs(magDiffs), pf1Percentile.value) * magDiffs.unit,
+    return Measurement(metric, np.percentile(np.abs(magDiffs.value), pf1Percentile.value) * magDiffs.unit,
                        extras=datums)

@wmwv
Copy link
Contributor Author

wmwv commented Feb 11, 2020

The one other use of numpy.percentile I found looks like

https://github.com/lsst/validate_drp/blob/7397373f0857cefcbb27b649c4ea122956b5991a/python/lsst/validate/drp/repeatability.py#L315

I think that one is fine as is.

@wmwv wmwv merged commit b33ae6f into master Feb 11, 2020
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.

None yet

2 participants