bmorris3 / tynt

Color filter approximations in Python
https://tynt.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
9 stars 4 forks source link

BUG: Astropy model cannot evaluate scalar input #9

Open pllim opened 4 years ago

pllim commented 4 years ago

Using the example from Getting Started:

>>> from tynt import FilterGenerator
>>> f = FilterGenerator()
>>> identifier = 'SLOAN/SDSS.rprime_filter'
>>> filt = f.reconstruct(identifier, model=True)
>>> filt.model(6000)  # BUG: Returns NaN
.../tynt/core.py:97: RuntimeWarning: invalid value encountered in true_divide
  return (mo - mo.min()) * tr_max / mo.ptp()
nan
>>> filt.model([6000, 6001])  # Works                                            
array([0.97632599, 0.        ])
bmorris3 commented 4 years ago

Hmm this is an interesting bug, do you mind if we brainstorm together on how to solve it?

(mo - mo.min()) * tr_max / mo.ptp() is a scaling factor meant to scale the transmittance curve such that the transmittance will roughly span zero to tr_max. But that was assuming the user always supplies wavelengths that span the full bandpass. As you've neatly demonstrated, this produced non-intuitive results when you supply wavelengths spanning a small fraction of the bandpass.

I thought at first that the way to solve this was to return the un-normalized transmittance at individual wavelengths by passing the wavelength argument to the model function through np.isscalar, but I don't like the idea of returning un-normalized transmittance curves in one case and normalized curves in the other.

Maybe the most rigorous (and expensive?) thing to do is to:

  1. Construct the model as a sum of Sine1D functions as we already do
  2. Actually evaluate m(wavelength) for the full range of wavelengths spanned by the bandpass, so we can scale the results appropriately with tr_max via the min and ptp calls
  3. Replace the lines which simultaneously evaluate and renormalize the curve with a simple call to the renormalized outputs.

I've implemented this in this branch for beginning the conversation.

pllim commented 4 years ago

Performance-wise, I am not sure if it is worth it to have the compound Sine1D model. It will never be as fast as interpolating the sampled transmittance (the other thing you store in Filter). It is interesting to have for academic curiosity but I don't see it being practical when you need to build an observation and that filter is just one of the 15 other components to evaluate.

Given that the original filter was already sampled, I think sampled -> FFT -> sampled is a nice circle anyway, no?

bmorris3 commented 4 years ago

Haha, I'm 99% sure I only implemented the astropy model framework because you asked for it. Though if your comment above succinctly supersedes your comment here, I agree! It took days to come up with the correct sinusoidal decomposition, but I'm happy to see it go. While being academically "neat", I can't think of a use-case where it's necessary.

pllim commented 4 years ago

Ah, ops! I did say "analytical"... 😅 Sorry to send you on a wild chase. 😞

It is still nice to have but maybe as an "aside" with caveats documented (e.g., need to use full wavelength range).

Thank you for the clarifications!