-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Added new magnitudes package, related docs, and tests. #1577
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
…ges to units.quantity and units.astrophys packages.
|
@pllim - thanks! Should this go in a |
|
Very exciting! One general comment: my guess would be that soon enough we will want to be able to convert between different magnitude systems, i.e., something like This will likely require a magnitude superclass, which contains a magnitude in a given system (just like p.s. The Maybe @mdboom can comment whether he had specific plans, as obviously that would be useful here. |
|
I cannot find |
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.
Nitpicky, but while STMAG is primarily used by HST, ABMAG is used much more broadly (almost all extragalactic work? certainly SDSS, etc.)
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 agree with @mhvk here. I think this can be made clearer if the section is titled something like "magnitude systems" and treats STMAG and AB mags as basically examples (albiet important ones).
|
Or maybe |
|
I'd wondered about |
|
@astrofrog - Will naming it @mhvk - I could rename |
|
Am I right that this only applies to spectral flux values? Do you have a plan for magnitudes in passbands? |
|
@kbarbary - Not in this PR. But the class is written to be expandable to include other magnitude systems. |
|
@mhvk I like your idea of a system argument, but I think it should be tied to a registry of The user would see the same thing: m = Magnitude(xxx, system='ab')
m = Magnitude(xxx, system='st')
m = Magnitude(xxx, system='vega')But underneath there would be a lookup registry of systems, like this: _magsystems = {'ab': ABMagSystem(),
'st': STMagSystem(),
'vega': SpectralMagSystem(<spectrum of vega>),
'bd17': SpectralMagSystem(<spectrum of bd+17>)}Here is a sample API for >>> ab = ABMagSystem()
# Spectral flux of a magnitude 0 object at specific wavelength
>>> ab.zpflux(4000.*u.AA)
<Quantity xxx erg / (cm2 s A)>
# Integrated flux of a magnitude 0 object in some_bandpass:
# Once calculated for a given bandpass, this value is cached within the MagSystem instance, so it is very fast.
>>> ab.zpbandflux(<some_bandpass>)
<Quantity xxx ph / (cm2 s)>The zpflux and zpbandflux methods above are the important ones. The rest is just log and division stuff that will be identical for every magnitude system. You could also have convenience methods that do the math for you, or this functionality could be in the # Convert flux to magnitude. These would return either a Magnitude object or a float.
>>> ab.flux_to_mag(1. * (u.erg / u.s / u.cm**2 / u.AA), 4000.*u.AA)
>>> ab.band_flux_to_mag(1. * (u.ph * u.s / u.cm**2), <some_bandpass>)
# Convert magnitude to flux. These would return Quantities.
>>> ab.mag_to_flux(..., 4000.*u.AA)
>>> ab.band_mag_to_flux(..., <some_bandpass>)It is very important that this "magnitude system" functionality be available outside of the |
|
@kbarbary - When spectrum or passband is involved, you need classes for them, which is out of the scope of this sub-package but will hopefully be handled by http://github.com/spacetelescope/pysynphot (in active development and not ready for review yet). |
|
@pllim Absolutely this depends on having I think it is important to think now about how the design would work with both spectra and bandpasses. |
|
@pllim BTW, what is the relation between pysynphot development and specutils? They seem to have overlapping planned functionality. |
|
@kbarbary - As regard to |
|
We are looking at that now. I think we will try to have it use the model framework much more directly in the astropy version we are developing, so there ought to be a very strong overlap of spectral models with what is in pysynphot. One could almost think of it now as extending models to handle spectral issues. I'm also going to try to get some discussion going on what spectral analysis priorities there are to help guide our other work in this area.
|
|
Real annoying point here: in other packages, we opt for verbosity. Can you change Mag to Magnitude wherever relevant? Also why all caps? |
|
@kbarbary Also also, I'm against string systems -- why not pass in the class itself? |
|
@adrn - don't agree with the argument against string systems -- I like how those can match attributes, as in |
|
@adrn I'm imagining that the m = Magnitude(xxx, system='vega')
m = Magnitude(xxx, system=SpectralMagSystem(<spectrum of Vega>))
m = Magnitude(xxx, system='ab')
m = Magnitude(xxx, system=ABMagSystem()) |
|
I guess I'm wondering why you need both? Shouldn't there be "one -- and preferably one -- way to do it"? It's a difference of one line: |
|
@adrn In your example, you'd have to load the spectrum of vega first, so it is actually more than one line. Using the string argument/registry, this would be taken care of behind the scenes. I admit that it is simpler for the AB system, where it would just be ab = ABMagSystem()
m = Magnitude(xxx, system=ab)The other thing is that there need only be a single instance of any given magnitude system. (Well there could be multiple instances, but it will only lead to inefficient code (they wouldn't share cached zeropoint vales for example). The registry would take care of this, so that multiple calls of In short, the preferable way to do it would be using strings, but for flexibility it would also accept |
|
A possible problem with m = Magnitude(1.*u.erg / u.s/u.cm**2/u.AA, system='ab')
m.vega # ??? can't convert from AB mags to Vega mags without knowing wavelength |
|
@kbarbary - I think your one-but-last point about I think that like for coordinates, it will be good to keep two worries separate: how simply to transform between flux and magnitude (this PR, as @pllim stressed), and how to deal with magnitude systems (a future PR; @pllim probably has ideas given her work on pysynphot). I guess in other words, there should be a basic magnitude which allows (and photon rates as well...) You are right, though, that this means the magnitude does need information about the wavelength, so it can convert one type of flux to the other (using @pllim: it would be very helpful to have an idea about what "system" magnitude API you are envisioning for synphot; do you have a link? Also, separately, to what extent is the current PR useful for pysynphot? Is it not too simplistic? |
|
Currently in As for my vision, I am hoping to move the simpler flux conversions to I am certainly open to suggestions. There seem to be a lot of good ideas so far in the comments already. |
|
I'll have some more specific comments here later, but just to address the location: I agree with @astrofrog that |
|
@aconley - my definitive source on everything tells me irradiance equals radiative flux [1]. So, still W/m2? |
|
@mhvk - Yes, yet another synonym, again in W m^-2. There's a welter of different systems here, and astronomers and radiometers often have different names for the same thing. That's why saying 'flux' is so confusing -- it is used to mean different things by different people in different communities. To a radiometer it should mean 'radiant flux', which is Luminosity to an astronomer. To a photometer, it's luminous flux (in lumens). To an astronomer, flux seems to mean 'whatever I'm talking about right now that has anything whatsoever to do with radiometry or photometry.' Flux density, as far as I know, is unique (although it has a synonym, spectral irradiance, for radiometers, and But, anyways, in practice, telescope pipelines return either 1) magnitudes, 2) counts, 3) photons, or 4) (spectral) flux densities. So those are the cases that are important to handle. 2 and 3 are special because they aren't universal -- you need more information about your telescope/detector/atmosphere, encapsulated as a zeropoint, to go anywhere with them. But you always know exactly how to go between, say, AB mags and a flux density. Irradiance/radiative flux is proportional to the thing you need for a broadband magnitude, but magnitude systems are cleverly arranged so that you don't need to know the constant of proportionality -- meaning you don't need to know the true system throughput, just it's wavelength dependence. The conventions around broadband flux density, as returned by telescope pipelines, are also set up so that you don't need to know the true throughput -- which is good, because you never know it! |
|
@mhvk - I agree with @mhvk that there's a lot of valuable discussion here. But I think it's valuable discussion mostly about a different, future PR (possibly in an affiliated package). There's a ton of valuable stuff to do here, but all I think should be in this actual PR is classes to represent the magnitude-to-physical quantity ratio conversion. This gives users something specific to use when they want to represent magnitudes or anything that's flux-like (+). Now to address @mhvk's
I can see the use here, and nothing of what I propose is incompatible with adding that later. My point here (and I think @astrofrog was saying the same?) is that sometimes you don't know the system. But you still want a way to represent the mags and fluxes, and switch between them. I promise I would use that literally today if it existed, and I know of at least one person in an office down the hall who asked me if such a thing exists in astropy (without any prompting). So I would say @pllim should remove the AB and STMAG part and any specific reference to systems, and leave the If so, then the remaining question is the (+) While in the original conversation we were talking about flux density (energy / time /area / spectral_unit), in fact what I'm saying above is that this generalizes to "fluxish" meaning any of the things astronomers sometimes use magnitudes to represent the ratio of. That is sometimes |
|
@eteq - Indeed, one sometimes does not know the units and one still would want to transfer. So, in my proposed API, one would use dimensionless as the underlying unit or even define a new onet. My larger point, however, remains, that the API could be much better, and as such I'm not in favour of merging this. |
|
I'll comb through this next week and re-implement I also want to point out that |
|
Just so we all know, this is definitely not going to make 0.3, as we decided on a feature freeze today in the interest of getting it out soon.
I don't understand how what @pllim has (especially with the modifications I suggest) is in conflict with your suggestion here. I'm saying we start with the simplest thing, and later on we can add support for additional systems. As far as I can tell, this could be done with units just as you suggest after this PR is merged. (I should add, I don't think we should use units to represent different magnitude systems for a few different reasons... But I think that belongs in a separate discussion, not this PR.)
I am advocating we use this as a starting point. I think it's terribly unwise to say we're going to wait until we've figured out the whole photometry system scheme. Long PRs with a lot of functionality in one have historically been much more work for everyone (and more prone to bugs), so I'm saying we should get the groundwork in place now, because even just that would be valuable right now for doing science in python. (and 👍 to EDIT: @mhvk - one thing that might clear things up: are you suggesting having only |
|
@eteq - you and @pllim certainly convinced me that we should keep photometric systems out of this PR, so let's indeed focus on a general magnitude. For those, too, I feel we need to think carefully about API (am I turning into @demitri?). My particular problems with the current API, in order of increasing generality:
Of course, I also realise one has to start somewhere, but it seems to me at least 1--3 should be addressed. I also think making use of To me, it would seem the best way to proceed is first construct a requirement list for magnitudes. [1] E.g., one can map p.s. On |
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.
Maybe just _builtin_zeropoint given that we use the word "zeropoint" elsewhere in the code?
|
One further thing that probably is obvious to most, but struck me again only now: one often thinks of magnitudes as dimensionless, since they are This would also suggest the |
|
@pllim, @eteq - I owe you both an apology -- I clearly had not looked carefully enough at the PR, it is much closer to what I envisioned than I thought. Clearly, I made too hurried a judgment. But at least this also meant it was relatively easy to implement all I thought would be nice to have, and hence I did a (very long) PR to your branch (pllim#1, or https://github.com/mhvk/astropy/tree/mag-math). It includes updated documentation and adapted tests (but not the many tests that will need to be added). With it, one can now do: EDIT -- with bright daylight came a better way to represent magnitudes; now changed below |
|
By introducing |
|
I thought it might be handy to provide a direct link to the documentation, so one can see how things should work: |
|
I (mostly) like what you've done here @mhvk! However, how will things work when we want to introduce magnitude systems like 'Vega'? I don't think we will be able to define a My solution is to simply replace your >>> mag = m.Magnitude(u.Quantity(3.63e-9, u.erg / u.cm ** 2 / u.s / u.AA),
... system=m.ST)
>>> mag
<Magnitude 0.0002334... ST>But |
|
@kbarbary - Thanks! I think a Note that in the unit-based scheme, one could still have Vega-based fluxes, but they might be in |
|
p.s. Just as a heads-up: I'm now working on my point (4), making this work for all logarithmic units rather than just magnitudes. The API is still similar, though the implementation somewhat different. The framework would be fairly easily extensible to other functional units. |
|
@mhvk - I need to consider the matter a bit more, I think, but there one thing in your API I find confusing
This seem opaque as to what's going on, and this is the use case I thought the API @pllim and I were talking about does with nicely; you have to actually type |
I think my example may have been somewhat misleading. In it, I tried to show how my proposal could be used as part of photometric calibration, where at some point one has an observed count rate of some reference star with a known magnitude in the Here, since the subtraction implies that units are divided, then the multiplication of the physical units means that Returning now to the question of how to store system zeropoints (rather than do calibration), the difference from what @pllim and you suggested is that in my proposal the zero point is simply defined by the physical unit, rather than being a log value corresponding to it. So, for the |
|
@eteq, you can close this. I can always build on the other PR as needed. Thanks. |
|
Ok, thanks for the work on this, anyway, @pllim ! |
|
@pllim - thanks also from me! |
|
You're welcome :) |
Added new
magnitudespackage. This handles basic conversion between magnitude and flux, and would be very useful forpysynphot. Also added related docs and tests. Updated existing docs where necessary, including a minor typo inQuantitydocstring. My Emacs is also set up to auto-delete trailing whitespace.p/s: This is based on idea from @eteq (see attached pic).