spacetelescope / synphot_refactor

Synthetic photometry using Astropy
http://synphot.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
39 stars 25 forks source link

Why throw away negative flux? #76

Closed pllim closed 8 years ago

pllim commented 10 years ago

A recent help call asked why is pysynphot (in ASTROLIB) automatically sets negative flux to zero, unless keepneg=True is explicitly given. Does someone knows the history of this decision?

I do not think pysynphot should be so judgmental. If the user wants to make a spectrum with negative flux, the user should be strong enough to bear the consequences. No?

vglaidler commented 10 years ago

It sets negative fluxes to zero when converting to magnitudes.

Because it doesn't make sense to convert a negative flux to magnitudes.

On 09/10/2014 09:46 AM, P. L. Lim wrote:

A recent help call asked why is |pysynphot| (in ASTROLIB) automatically sets negative flux to zero, unless |keepneg=True| is explicitly given. Does someone knows the history of this decision?

I do not think |pysynphot| should be so judgmental. If the user wants to make a spectrum with negative flux, the user should be strong enough to bear the consequences. No?

— Reply to this email directly or view it on GitHub https://github.com/spacetelescope/pysynphot/issues/76.

pllim commented 10 years ago

Thanks for the clarification, @vglaidler .

Supporting spectrum objects with fluxunits set to magnitudes in astropy is another headache. I am tempted to just not support them, since I have not seen anyone use it yet. You can still calculate spectrum properties in magnitudes without having the object flux attribute itself to be in magnitudes. But, that should be discussed in a separate thread (in the future when the time is right).

vglaidler commented 10 years ago

In pysynphot, .fluxunits is an attribute that is specifically used to determine the units in which the .flux property should be returned. It doesn't determine the internal flux units. Similarly, on construction, the fluxunits keyword specifies the units of the provided flux array.

Magnitudes are frequently used in both cases.

On 09/10/2014 10:10 AM, P. L. Lim wrote:

Thanks for the clarification, @vglaidler https://github.com/vglaidler .

Supporting spectrum objects with |fluxunits| set to magnitudes in |astropy| is another headache. I am tempted to just not support them, since I have not seen anyone use it yet. You can still calculate spectrum properties in magnitudes without having the object flux attribute itself to be in magnitudes. But, that should be discussed in a separate thread.

— Reply to this email directly or view it on GitHub https://github.com/spacetelescope/pysynphot/issues/76#issuecomment-55120229.

pllim commented 10 years ago

Just for the record, astropy/astropy#1894 attempts to address magnitude units in astropy.

pllim commented 10 years ago

Actually, won't it be more transparent if pysynphot simply raises an error if user attempts to convert negative flux to magnitudes? I am not saying that we should change this in ASTROLIB, I am just asking for the astropy implementation.

vglaidler commented 10 years ago

That was the original implementation. It was removed at the request of astronomers because astronomers frequently have real spectra which really have negative values because of noise, sky subtraction, & so forth. Discussion with the astronomers who commissioned pysynphot determined that zero clipping, with a warning, was the appropriate action to avoid intensely annoyed users. :)

(Of course, it should be a real warning, not a print statement.)

On 09/10/2014 10:57 AM, P. L. Lim wrote:

Actually, won't it be more transparent if |pysynphot| simply raises an error if user attempts to convert negative flux to magnitudes? I am not saying that we should change this in ASTROLIB, I am just asking for the |astropy| implementation.

— Reply to this email directly or view it on GitHub https://github.com/spacetelescope/pysynphot/issues/76#issuecomment-55127866.

pllim commented 10 years ago

Good to know the history behind this. Thanks!