Fix gauss total_flux unit handling #154
Conversation
DOC: Update GaussianFlux1D examples
pllim
force-pushed
the
fix-gauss-totalflux-units
branch
from
00ae6a1
to
d0ea38e
Compare
| # happen here. | ||
| tf_unit = u.erg / (u.cm * u.cm * u.s) | ||
| if isinstance(total_flux, u.Quantity): | ||
| total_flux = total_flux.to(tf_unit) |
There was a problem hiding this comment.
Could this conversion to tf_unit include an equivalency parameter so that total fluxes can be provided in units of, e.g., photons/s/cm2 or Jy Hz?
There was a problem hiding this comment.
@mkelley , I thought about that but I wasn't sure if using u.spectral_density(mean) would give the correct conversion. If you think that is correct, then I can certainly add it in.
There was a problem hiding this comment.
No, wait. I take it back. I can't use spectral_density because it is integrated flux unit.
There was a problem hiding this comment.
Ah, yes. I was thinking there was an equivalency for flux.
There was a problem hiding this comment.
How about we merge this as is and see how things play out over at astropy/astropy#7593?
|
Except for that comment, the changes all make sense to me. Thanks for the prompt fix. |
|
@mkelley , do you need an immediate bug fix release or can you wait? If you can wait, is there a deadline? Thanks for the review! |
|
There's no rush for me. I can use the amplitude normalized gaussian just fine. |
Fix gauss total_flux unit handling
Fix #153
@mkelley , are you interested to review before I merge?