-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Function units: mag, dB, and dex #1894
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
Conversation
astropy/units/utils.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use hasattr(value, 'imag') as you do in sanitize_scale()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After implementing it, I thought again: the reason for the try/except is that the complex case is only needed for the corner case of u.mag**0.5 (it really belongs in #1628; will move it there). So, my goal is to ensure this impacts the normal cases as little as possible; changing it to hasattr increases the time for is_effectively_unity from 261 to 433 ns). In contrast, for sanitize_scale, the hasattr is the fastest, since otherwise one is attempting a calculation that usually is meaningless. (I'll add some comments to ensure this is clear in the code)
|
Looks powerful. :) Is it possible to convert 17 STmag to ABmag? It looks possible but I do not see a clear example in the iPython Notebook (or maybe I missed it). |
|
@pllim - Thanks! And good question about ST->AB. I hadn't thought about this, since that needs a wavelength, but of course we do have the appropriate equivalency, so it might just work... I'll admit that I didn't check the answer is actually correct, but the above shows that at least for a wavelength near EDIT -- since it is so neat that it just works, I updated the notebook view to include this conversion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I tried to base my functional units on UnitBase (or CompositeUnit), but I found I needed to make lots of changes to core to get it to work (e.g., in all of the arithmetic; much cleaner if normal units just do not know how to deal with functional ones).
Hence, functional units are is not a UnitBase subclass, but for the equivalencies they have to behave like a unit in some respects. So, I'm trying to ducktype it, but I thought it was best to limit that to only a few places, hence here I use that Unit will very quickly pass through unchanged something that it considers to be a unit already.
In the end, I think it may be better to have a minimal Abstract Base Class for units; this may also be good if we ever would like to get the different python units packages to talk to each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I think what's here probably makes sense for now.
|
Update 22 Sep 2014: No longer an issue I'll start collecting a list here of things that are not yet quite OK:
|
astropy/units/functional/core.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdboom - equivalencies get assigned here. Note also that tunit=self; this is the reason the check of the equivalencies had to be adapted. Still wonderful how well it works, though!
|
Hey guys, I'm in need of the |
|
@wkerzendorf - just to be clear, here you mean you wanted #1628, is that right? Or you meant you wanted this PR, which adds the capability to switch between e.g. logarithmic and linear units? (incidentally, this also reminds me I was going to give feedback on this to @mhvk ... will try to do so soonish!) |
|
I'd like to be able to write down a unit as |
|
@wkerzendorf - yes, for To do list
|
|
Once this has documentation, we should send it to the list for review since it is a pretty big feature. |
astropy/units/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ABMag (or STMag)? Are they not supposed to be "public"? I can perhaps see STMag as being moderately domain-specific, but ABMag seems quite common to me (also see my comment in the discussion on where the API should "live")
|
@mhvk - generally I like this approach! But I do have some questions/concerns:
It seems wrong to me that
And I agree with @astrofrog that once we've settled enough that it can be documented, this is a list-worthy change. |
|
@mhvk - I'm scheduling this for 1.0 since it's likely not going to be ready for 0.4, but feel free to close if it's been superseded elsewhere. |
0c97841 to
fede5de
Compare
|
@eteq, @mdboom - I rebased once more, and, as you'll have seen, sent off a request on astropy-dev for further review. I hope you will have time as well. Any ideas about the larger issues mentioned above (#1894 (comment)) would be welcome as well (though hopefully for a separate PR, this one is big enough as is!). |
|
Looks like this will be a great addition to I agree with @eteq above that "functional unit" is confusing terminology, but it looks like it only appears in the code, and not in the documentation (unless directly referencing the code) -- is that right? Would it be less confusing to change from Comments on the documentation:
|
|
@PBarmby - thanks! I have now replaced any remaining instances of "functional unit" with "function unit". It may still be a little confusing to have I should perhaps add that the reason I started like this was that Thanks also for the comments on the documentation:
|
|
In the spirit of the mailing list discussion, I removed myself as "assignee". @mdboom - I fear this would be one for you... (though, @eteq, I'd definitely value further comments from you as well). Both: see #1894 (comment) for things that maybe should become separate issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdboom's #3554 (comment): add period.
Function units: mag, dB, and dex
|
Nice to see it in! It does mean i better get going with getting these units written to fits, etc. So, here again my list of leftovers, but now with corresponding PRs:
|
|
Sorry -- I skimmed through the PR and missed that there was still work left to do. I can hit the big "revert" button if you'd rather not be rushed about the remaining things... |
|
I actually think it is fine to have it in as is; this PR was rather big and therefore unwieldy. The most important follow-up is that which makes using the units easier by doing the parsing. While this is not essential, I think it is important enough to help users that I set the milestone for #3554 to 1.10. The other PRs are icing on the cake or restructuring, so they can be a bit slower. |
Now that
UnitandQuantityhave calmed down a bit, something a bit bigger: introducing a framework for functional units and an application to logarithmic units, so that one do things likedex(cm/s**2),dB(mW), andmag(AB).I think the usage I have in mind is best illustrated through an example -- see the output from an ipython-notebook session using magnitudes on http://www.astro.utoronto.ca/~mhvk/functional_units_magnitudes.html
For here, just a very brief outline:
I hope initially to get feedback mostly on whether this covers what usage people can foresee.
Some notes:
Magnitudeclass of Added new magnitudes package, related docs, and tests. #1577. In practice, the idea is not dissimilar; the main difference is that rather than store a zero-point magnitude, the unit corresponding to mag=0 is stored (or, equivalently, what one divides by before taking thelog).generic(i.e., no FITS, etc.)UnitBase, as that required much larger changes tounits(see my earlier trial, https://github.com/mhvk/astropy/tree/units-functional). However, an abstract base class with expectations of what any unit should provide might be helpful. Technical advice very welcome!dex,mag, anddB(without underlying physical unit).