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

DM-14548: Make dimensionless quantities in refraction floats #354

Merged
merged 2 commits into from May 23, 2018

Conversation

isullivan
Copy link
Contributor

Also fix return value docstring.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

A few minor requests and one question, but overall looks fine. Thank you for the quick fix.

densityFactor = (1. + eqn)*(dryPressure/cds.mbar)/(temperature/units.Kelvin)
eqn = float((dryPressure/cds.mbar)*(57.90E-8 - 9.3250E-4*units.Kelvin/temperature +
0.25844*units.Kelvin**2/temperature**2.))
densityFactor = (1. + eqn)*float(dryPressure/cds.mbar)/float(temperature/units.Kelvin)
Copy link
Contributor

Choose a reason for hiding this comment

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

From reading the astropy docs I expected to see value.to_value(units), instead of using float(value/units). For example:

dryPressure.to_value(cds.mbar)

and this seems a clear way to express what you want done.

If what you've done is a standard way to get a float in particular units, can you please point me to the appropriate docs? I realize it works, but based on reading what I found, it doesn't seem obvious to me that it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that looks more clear. I'll change to to_value(units) throughout.

@@ -212,8 +212,8 @@ def humidityToPressure(weather):

Returns
-------
`float`
The water vapor pressure in millibar
`astropy.units.quantity.Quantity`
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest astropy.units.Quantity since that is the documented version. The existence of a quantity sub-module appears to be an internal detail of astropy.

@isullivan isullivan merged commit 6268be7 into master May 23, 2018
@ktlim ktlim deleted the tickets/DM-14548 branch August 25, 2018 06:44
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