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

Split noise injection out of `SimulatedRainbow` into its own `.inject_noise` action. #107

Closed zkbt closed 2 years ago

zkbt commented 2 years ago

For some applications, it will be useful to create an entirely noiseless SimulatedRainbow. Right now, the only way to do that is with a signal_to_noise=np.inf, but that feels like a bit of a kludge. Let's split the noise injection out into its own separate action, so what has previously been

SimulatedRainbow(signal_to_noise=100).inject_transit()

might now become

SimulatedRainbow().inject_noise(signal_to_noise=100).inject_transit()

This would have the added benefit that we could do cleverer things with getting the noise more accurate for high-amplitude signals (like eclipsing binaries), where the fractional noise should increase when the stellar flux is low.

A lot of code relies on SimulatedRainbow, so we should make sure to alert all current users and update the documentation very clearly!

zkbt commented 2 years ago

@catrionamurray, @Pat-Wachiraphan , @will-waalkes, I think you're the folks who are mostly likely to have some code floating around that depends on SimulatedRainbow automatically generating some noise for each flux value. I'd like to change things so that SimulatedRainbow generates a noiseless dataset, but we can inject noise into it very easily with a .inject_noise() function (see above).

The reasons for this are (a) I think it makes more sense alongside the .inject_transit and .inject_systematics functions and (b) I think Molly can add some features and options to the noise injection and I'd rather her not have to worry about all of the other complicated stuff going on in the initial initialization of the SimulatedRainbow object.

My question for you is "would you please be OK with us making this change?" It might temporarily break some of your code, but I think you could fix many problems with a one-time global find and replace of SimulatedRainbow() with SimulatedRainbow().inject_noise(), and hopefully the others are too painful to fix? I would develop this on a branch and warn you again before merging it into develop, but I'd like your approval before I start down that path! Thanks!

catrionamurray commented 2 years ago

Sounds good to me!

will-waalkes commented 2 years ago

Yep that sounds good

Pat-Wachiraphan commented 2 years ago

Sound good to me as well!

zkbt commented 2 years ago

Awesome. Thanks, all! I'm on it!

zkbt commented 2 years ago

Closed by #119 .

@catrionamurray , @will-waalkes , @Pat-Wachiraphan , @kortizceballos , @mone0982 please be warned! From version 0.1.8 onward SimulatedRainbow() won't inject its own noise automatically. You'll instead need to do something like SimulatedRainbow().inject_noise(). Good luck!

will-waalkes commented 2 years ago

Cool!