Conversation
arunkannawadi
left a comment
arunkannawadi
left a comment
There was a problem hiding this comment.
LGTM! I think this is the case, but can confirm that trying to thin an already thinned SED (for the given bandpass) is trivial the second time?
| assert thin_err2 < 1.e-3 | ||
| np.testing.assert_allclose([thin_s2.blue_limit, thin_s2.red_limit], | ||
| [s.blue_limit, s.red_limit]) | ||
|
|
There was a problem hiding this comment.
It'd be nice to have a test where you thin the SED without passing the bandpass and integrate over a bandpass, and compare that against the result with the bandpass passed in.
There was a problem hiding this comment.
That's a good idea. There aren't actually any guarantees about the relative performance with vs without the bandpass argument. Mostly the useful thing was truncating the range to that of the bandpass. But I did add a comparison to a call without bandpass. The error is slightly larger than when it uses the bandpass during thinning, which is probably often true, but not required.
I don't think that's necessarily true. The goal of thinning is to find a smaller number of points that give the same integral for the flux (normally bolometric; here for a given bandpass) as the input sampling. If you repeat the process, it would be comparing to the first-pass thinned SED, so it could in principle remove more points. We also don't exhaustively search for all possible removals that are consistent with the requested tolerance, so the first result isn't "optimally thin" by any definition. So it's even possible that the second pass result could remove points and also be within tolerance to the original. |
2 participants
For some Piff stuff, I wanted to be able to thin an SED targeting a particular bandpass, rather than generically. Especially to have the output limits truncated to the bandpass limits, but also targeting the integral for thinning to be specifically the one you get with this bandpass.
This PR adds that functionality.