search

zkbt / chromatic

Tools for visualizing spectrosopic light curves, with flux as a function of wavelength and time.
MIT License
14 stars 4 forks source link

add inflate_uncertainty action #174

Closed Pat-Wachiraphan closed 2 years ago

Pat-Wachiraphan commented 2 years ago

I added inflate_uncertainty action to inflate uncertainty from files to photon noise (flux scatter).

zkbt commented 2 years ago

This looks great, @Pat-Wachiraphan ! Can I add one request? In the best case scenario, the original expect uncertainties have been estimated from (basically) the number of photons we detected from the star, and then some mysterious sources of systematic noise sources (which hopefully are small) are introducing extra scatter on top of that. So, in principle, we should never deflate the uncertainties below their original values; the only time we should multiply the original uncertainties by a number less than 1 would be if the expected uncertainties have been catastrophically overestimated for some deep problem in the pipeline that was being used. (That's still happening in some of the datasets, I think, so we shouldn't rule out the possibility that it should be necessary, but ideally we should generally not let the inflation factor go below 1.)

So, could we please include a default limit that the minimum inflation inflate_ratio should be 1 (I think using np.minimum for this should work)? Or, given that some pipelines are still improving, maybe include a keyword argument for the minimum value that you want to let inflate_ratio be, which defaults to 1?

Pat-Wachiraphan commented 2 years ago

This looks great, @Pat-Wachiraphan ! Can I add one request? In the best case scenario, the original expect uncertainties have been estimated from (basically) the number of photons we detected from the star, and then some mysterious sources of systematic noise sources (which hopefully are small) are introducing extra scatter on top of that. So, in principle, we should never deflate the uncertainties below their original values; the only time we should multiply the original uncertainties by a number less than 1 would be if the expected uncertainties have been catastrophically overestimated for some deep problem in the pipeline that was being used. (That's still happening in some of the datasets, I think, so we shouldn't rule out the possibility that it should be necessary, but ideally we should generally not let the inflation factor go below 1.)

So, could we please include a default limit that the minimum inflation inflate_ratio should be 1 (I think using np.minimum for this should work)? Or, given that some pipelines are still improving, maybe include a keyword argument for the minimum value that you want to let inflate_ratio be, which defaults to 1?

Implemented!

zkbt commented 2 years ago

Thanks, @Pat-Wachiraphan ! I'm merging this into develop now. I'll get it up to the pip version probably later this week.