Skip to content

Quantity throws an unrelated warning when converting through equivalencies? - #779

Merged
embray merged 1 commit into
astropy:masterfrom
embray:issue-779
Feb 15, 2013
Merged

Quantity throws an unrelated warning when converting through equivalencies?#779
embray merged 1 commit into
astropy:masterfrom
embray:issue-779

Conversation

@embray

@embray embray commented Feb 14, 2013

Copy link
Copy Markdown
Member

I just noticed this one:

>>> x = 50.*u.THz
>>> x.to(u.eV, equivalencies=u.spectral())
WARNING: Converting Quantity object in units 'J s' to a Numpy array [astropy.units.quantity]
<Quantity 0.206783375651 eV>

It seems to get the conversion right, but it looks like the issue is deep in the units code for converting with equivalencies -- it looks like somewhere _si.h is getting converted into an array, but I don't see where (from a quick dig through the code). Maybe @mdboom knows where this could be?

@astrofrog

astrofrog commented Feb 14, 2013

Copy link
Copy Markdown
Member

I'm tagging this as 0.2.0, but maybe it's not critical enough, so it can be re-tagged as 0.2.1 if desired.

@mdboom

mdboom commented Feb 14, 2013

Copy link
Copy Markdown
Contributor

This seems to be a byproduct of the recent constants-as-quantities changes. In the spectral equivalency, there is an entry:

   lambda x: _si.c / x

Since a constant divided by a float gives us a Quantity, unit.to now returns a Quantity. When trying to instantiate a Quantity from another Quantity as the value, this warning is raised.

There are a number of ways to plug this hole, but I'm not sure which is the best. 1) Unit could be patched to return Quantity.value if it somehow ends up with a Quantity to return. 2) Quantity could be patched to allow for instantiation from another Quantity (not the best solution, because it would throw out the unit). 3) The equivalencies can use some_constant.value rather than some_constant. There's a certain correctness here, because equivalency mappings need to operate outside of the scope of units (they deliberately break unit boundaries), so they really shouldn't be involving units.

Any other thoughts?

@eteq

eteq commented Feb 14, 2013

Copy link
Copy Markdown
Member

I think I prefer # 3 (in this specific case that means changing the code you mentioned to lambda x: _si.c.value / x, right?). I don't see any downside, and as you say it seems like they shouldn't care.

Although it might also make sense to change this to lambda x: constants.c.si.value / x - that's using the constants API as it's now intended. I suppose the result is the same, thoguh.

@embray

embray commented Feb 14, 2013

Copy link
Copy Markdown
Member

Yeah, this should just say c.value. Anything internal that just wants the "value" of a constant should use that, instead of trying to use it like a Quantity. (previously the ConstantDefinition class was a subclass of float for a while, which is why there are a few things like this--I thought I had gotten them all but apparently not.

@embray
This should fix astropy#779. As @mdboom explained these equivalencies…
19a209b 
… should be using the value of c (in m/s) as a scaling factor, but its units shouldn't really come into it. This should have been changed to c.value and h.value as part of astropy#745.
@embray

embray commented Feb 14, 2013

Copy link
Copy Markdown
Member

How's this attached fix look?

@adrn

adrn commented Feb 14, 2013

Copy link
Copy Markdown
Member Author

Well, that looks simple enough!

@eteq

eteq commented Feb 15, 2013

Copy link
Copy Markdown
Member

seems fine to me

@embray

embray commented Feb 15, 2013

Copy link
Copy Markdown
Member

Segfaults in Travis? I don't even.... 😡 I'm going to go ahead and safely ignore that and merge this.

embray added a commit that referenced this pull request Feb 15, 2013
@embray
Merge pull request #779 from iguananaut/issue-779
3a46b77 
Quantity throws an unrelated warning when converting through equivalencies?
@embray
embray merged commit 3a46b77 into astropy:master Feb 15, 2013
@embray
embray deleted the issue-779 branch February 15, 2013 15:28
embray added a commit that referenced this pull request Feb 15, 2013
@embray
Merge pull request #779 from iguananaut/issue-779
f75813b 
Quantity throws an unrelated warning when converting through equivalencies?
@eteq

eteq commented Feb 15, 2013

Copy link
Copy Markdown
Member

@iguananaut the current master branch seems to still be giving the same segfault, but only for numpy 1.4 . I'm checking it now on a local copy of 1.4

keflavich pushed a commit to keflavich/astropy that referenced this pull request Oct 9, 2013
@embray
Merge pull request astropy#779 from iguananaut/issue-779
0380614 
Quantity throws an unrelated warning when converting through equivalencies?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@embray @astrofrog @mdboom @eteq @adrn