Skip to content
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

BUG: Astropy model cannot evaluate scalar input #9

Open
pllim opened this issue Apr 28, 2020 · 4 comments
Open

BUG: Astropy model cannot evaluate scalar input #9

pllim opened this issue Apr 28, 2020 · 4 comments

Comments

@pllim
Copy link

pllim commented Apr 28, 2020

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
Copy link
Owner

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
Copy link
Author

pllim commented Apr 30, 2020

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
Copy link
Owner

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
Copy link
Author

pllim commented Apr 30, 2020

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants