Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Dec 11, 2013

Now that Unit and Quantity have 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 like dex(cm/s**2), dB(mW), and mag(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:

In [1]: import astropy.units as u, astropy.units.functional as lu
In [2]: iv = lu.Magnitude(100. * u.ct/u.s)
In [3]: iv, iv.quantity, iv.unit
Out[3]: (<Magnitude -5.0 mag(ct / s)>, <Quantity 100.0 ct / s>, MagUnit('ct / s'))

In [4]: iv.unit.physical_unit, iv.unit.functional_unit
Out[4]: (Unit("ct / s"), Unit("mag"))

I hope initially to get feedback mostly on whether this covers what usage people can foresee.

Some notes:

Copy link
Member

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()?

Copy link
Contributor Author

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)

@pllim
Copy link
Member

pllim commented Dec 11, 2013

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).

@mhvk
Copy link
Contributor Author

mhvk commented Dec 11, 2013

@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...
...
It does!!

In [1]: import astropy.units.functional as lu; import astropy.units as u
In [2]: mst = 5 * lu.STmag
In [3]: mst.to(lu.ABmag, equivalencies=u.spectral_density(400*u.nm))
Out[3]: <Magnitude 5.681751800680007 mag(AB)>
In [4]: mst.to(lu.ABmag, equivalencies=u.spectral_density(550*u.nm))
Out[4]: <Magnitude 4.9902383098486 mag(AB)>

I'll admit that I didn't check the answer is actually correct, but the above shows that at least for a wavelength near V it gives roughly the same magnitude; since internally this just converts to STmag -> flux-in-wavelength-units -> flux-in-frequency-units -> ABmag, it should be fine.

EDIT -- since it is so neat that it just works, I updated the notebook view to include this conversion.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@mhvk
Copy link
Contributor Author

mhvk commented Dec 12, 2013

Update 22 Sep 2014: No longer an issue

I'll start collecting a list here of things that are not yet quite OK:

  • multiplication of unit and functional unit (e.g., u.m * lu.MagUnit(u.ct/u.s)) should fail (reverse fails correctly)

Copy link
Contributor Author

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!

@wkerzendorf
Copy link
Member

Hey guys, I'm in need of the dex unit. Will this PR be merged soon?

@eteq
Copy link
Member

eteq commented Mar 24, 2014

@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!)

@wkerzendorf
Copy link
Member

I'd like to be able to write down a unit as 'dex(lsun)' or 'log10(lsun)' - @mhvk I think pointed me to this PR.

@mhvk
Copy link
Contributor Author

mhvk commented Mar 24, 2014

@wkerzendorf - yes, for dex with a unit, you'll need this PR. @eteq - comments would be most welcome! Also on the API, somewhat implicitly defined by the ipython notebook example that I gave at the top.

To do list

  • ensure dex can be output to FITS and CDS (dB and mag are not possible with the current standards)
  • add documentation (likely following above notebook)

@astrofrog
Copy link
Member

Once this has documentation, we should send it to the list for review since it is a pretty big feature.

Copy link
Member

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")

@eteq
Copy link
Member

eteq commented Apr 7, 2014

@mhvk - generally I like this approach! But I do have some questions/concerns:

  • What is the long-term plan for connecting w/ non-functional quantities? In your example, if I'm understanding correctly, you autmatically "upgrade" a regular quantity to a functional quantity where the units are consistent, right? E.g., Magnitude(1*u.cnt/u.s) + 1*u.mag yields <Magnitude 1.0 mag(ct / s)>, so the 1*u.mag was simply assumed to be in u.cnt/u.s. That makes sense for your example, but I'm not sure it's reasonable in other functional-unit scenarios, so I'm just trying to understand how general you mean that to be (perhaps it should be sort of "opt-in" for units where it makes sense?).

    Along related lines, I'm a bit confused by the process of sending functional quantities back to regular quantities. Specifically, I found this confusing:

>>> q = 1*u.mag
>>> q * u.arcsec**-2
<Quantity 1.0 mag / arcsec2>
>>> fq = u.Magnitude(1*u.count/u.s) + 1*u.mag
>>> fq
<Magnitude 1.0 mag(ct / s)>
>>> fq * u.arcsec**-2
  UnitsError: Cannot multiply functional quantities with anything unless they are dimensionless
>>> fq.functional_value * u.arcsec**-2
<Quantity 1.0 mag / arcsec2>

It seems wrong to me that fq * u.arcsec**-2 didn't work, and of course it's crucial for any work that's resolved, as that's standard surface brightness units... shouldn't it just behave like a regular u.mag quantity?

  • I noticed in the current PR you have only a subset of the items in functional imported in units/__init__.py. Do you mean for only those to be part of the "public" API, and the rest is private? Or is everything in functional supposed to be part of the API? I'm rather against the latter case, because I think it'll be confusing to have two separate namespaces for units/quantities that are meant to be used together.
  • I find myself reading the phrase "functional unit" or "functional quantity", and thinking "does that mean the regular unit and quantity classes are non-functional?" This is just because English has weird vocabulary, but there it is nonetheless... So maybe just something like FunctionUnits and FunctionQuantity instead? Or perhaps changing the terminology a bit further to something like NestedUnits (that is, a u.ct/u.s quantity is "nested" inside a u.mag quantity is another way of thinking about this).
  • I like the fact that I can do fq.to(u.ct/u.s) and get out an answer that makes sense. The opposite does not work, though: (1*u.ct.u.s).to(u.mag) does not work. I'm glad that that's the case, but can you just clarify if that route is intended to be permanent? That is, is it supposed to be that the only way to get e.g. magnitude functional quantities from a unitful quantity is to do u.Magnitude(...), with no .to channel?
  • A more specific mag-related item: right now I think you're assuming terminology such that "magnitude" = -2.5*log10(value) . The trouble is, there's some subtly different definitions of "magnitude". The main example I'm thinking of is SDSS: they use ASinh magnitudes, a system that limits to the standard ("Pogson", they call it) magnitude system, but behaves differently at flux<~0. It would be nice for us to implement that (certainly it would be useful for me, as I use the SDSS a lot), but more important is that it be possible to implement in the future. I'm a bit unclear if that's an issue or not: will it simply require a new AsinhMagUnit and AsinhMagQuantity with the appropriate functions linking them to their quantity unit, or has this now forced the situation so that u.mag has to be the Pogson mag, and can't be asinh mags?

And I agree with @astrofrog that once we've settled enough that it can be documented, this is a list-worthy change.

@astrofrog
Copy link
Member

@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.

@astrofrog astrofrog added this to the v1.0.0 milestone May 9, 2014
@mhvk
Copy link
Contributor Author

mhvk commented May 9, 2014

@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.

Yes, that is fine. I will take into account @eteq's comments and then ping people for further review.

@mhvk mhvk force-pushed the units-functional3 branch from 0c97841 to fede5de Compare February 24, 2015 20:21
@mhvk
Copy link
Contributor Author

mhvk commented Feb 24, 2015

@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!).

@mhvk mhvk added this to the v1.1.0 milestone Feb 24, 2015
@mhvk mhvk self-assigned this Feb 24, 2015
@PBarmby
Copy link
Contributor

PBarmby commented Feb 26, 2015

Looks like this will be a great addition to units!

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 astropy.units.function to astropy.units.log ?

Comments on the documentation:

  • generally very clear and well-done!
  • do ST and AB mag systems need a definition reference?
  • in the example showing computation of reddening, would it be useful to explain why one of the 3 values at the end is Magnitude and the other 2 are Quantity
  • the example after "Note that one can take the automatic unit conversion quite far," while very nifty, may overwhelm a new user.
  • the "Numpy functions" section should more explicitly spell out which functions will work (statistical only?)

@mhvk mhvk changed the title Functional units: mag, dB, and dex Function units: mag, dB, and dex Feb 27, 2015
@mhvk
Copy link
Contributor Author

mhvk commented Feb 27, 2015

@PBarmby - thanks! I have now replaced any remaining instances of "functional unit" with "function unit". It may still be a little confusing to have astropy.units.function instead of astropy.units.logarithmic, but in practice one would anyway just do from astropy.units import Magnitude.

I should perhaps add that the reason I started like this was that units/core.py has a comment "TODO: Support functional units, e.g. log(x), ln(x)" -- so I assumed there were other functions that one might use. @eteq gave on example of something actually used in astronomy: the SDSS magnitudes which are defined using an asinh function (but which this PR does not yet support).

Thanks also for the comments on the documentation:

  • Added references and also gave STmag and ABmag their own docstring.
  • Now explicitly tell why the extinctions are quantities rather than magnitudes.
  • Agreed that the final part may be too much for starting users, but I hope it is OK given that I introduce it as a separate note.
  • Realised it is actually not that easy to explicitly list numpy functions and methods that are allowed/disallowed, and I worry that any list will quickly be out of data. Instead, I now added a note asking users to let us know if things work in unexpected ways.

@mhvk mhvk removed their assignment Mar 20, 2015
@mhvk
Copy link
Contributor Author

mhvk commented Mar 20, 2015

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.

Copy link
Contributor Author

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.

@mhvk mhvk mentioned this pull request Jun 5, 2015
mdboom added a commit that referenced this pull request Jun 5, 2015
Function units: mag, dB, and dex
@mdboom mdboom merged commit 9f81035 into astropy:master Jun 5, 2015
@mhvk
Copy link
Contributor Author

mhvk commented Jun 5, 2015

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:

  1. The function units need to connected to the string readers so that u.Unit('mag(Jy)') would return a FunctionUnit. --> see Enable parsing of mag, dB, dex #3554.
  2. Should support output to fits and vounit for the subset of all possible logarithmic units for which it is possible. --> Functional units in FITS and VOTable #3822
  3. I had to do some duck-typing in UnitBase and Unit to ensure that function units get recognised when they should be, but not when the code expects a normal unit. A minimal abstract base class for units would be a more elegant solution. --> Consider Abstract Base Classes for Quantity and Unit #3650

@mhvk mhvk deleted the units-functional3 branch June 5, 2015 18:13
@mdboom
Copy link
Contributor

mdboom commented Jun 5, 2015

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...

@mhvk
Copy link
Contributor Author

mhvk commented Jun 5, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants