mzechmeister / viper

GNU General Public License v3.0
5 stars 3 forks source link

class spectrum #28

Open mzechmeister opened 1 year ago

mzechmeister commented 1 year ago

Just some thoughts about a new spectrum class. Probably too fancy and too object orientated.

Note the inst files already have the Spectrum method for spectral data https://github.com/mzechmeister/viper/blob/41cdd82c96e09124984b7466f396d74404bf18e6/inst/inst_TLS.py#L23 and spectral models are handled as anonymous function https://github.com/mzechmeister/viper/blob/41cdd82c96e09124984b7466f396d74404bf18e6/viper.py#L276

The new class spectrum of course has attributes like

star.flux
star.wave
star.lnwave

Slicing the spectrum could slice everything (flux and wave)

star[idx_ok]

Could be done via __getslice__ or as a subclass of recarray. I think, recarray was somewhat slower.

Multiplication (__mul__) and addition (__add__) of two spectra would act only flux

star * cell
star1 + star2
res = star - starmod

Well, if the boundary and sampling is different, it gets tricky. Also fancy, multiplication with a callable function

star * np.poly1d([1, 2, 3])

More for fun, Doppler shift a spectrum with the shift operators

star << 5.4

Convolve with a kernel (__matmul__)

star @ gauss

Thus the forward model in https://github.com/mzechmeister/viper/blob/41cdd82c96e09124984b7466f396d74404bf18e6/model.py#L93 could be written as

Sj_eff = self.envelope * (self.IP @ ((self.S_star << v) * self.cell))

Interpolate spectrum (thus implicitly treated as a model) via __call__ (see also https://github.com/mzechmeister/serval/blob/620156a7346af8ca6fc1e91dec8aba1522b019b0/src/serval.py#L224)

star(lnwave_wave, 'cubic')

More methods having an operation: https://docs.python.org/3/reference/datamodel.html?highlight=__add__#emulating-numeric-types.

While important lines looks nicer, I think, there are too few of those lines. So at the end we have even more code. Also I expect entering the class methods all time during fitting creates overhead and will slow down the code.

JanaKoehler1985 commented 1 year ago

Uff, ok. Well, during our discussions yesterday I also had for a short moment the idea of entering a class for all our spectra. After I spend already yesterday a few hours on starting to rename the variables, and by realizing I will spend the next days even more time on it, I didn't like the idea not so much anymore. The problem is, when you start doing this big chances on an existing code, it causes a lot of work and you have to be sure , everything is still working in the end. Ok, entering a class and saving the spectra as part of that will be easy, but it goes through all the scripts by using it in the right way. Even all of that looks fancy and interesting, I am not sure if we would do ourself a favor with it. As you just said it in the end,the question is how much we would win by doing that. Still we could think about, if at least part of that could be useful and doable.

mzechmeister commented 1 year ago

Class spectrum or stellar spectrum could take care of BERV computation. It could be done in a class (stellar) Spectrum/observation. The inst function Spectrum might inherit from the class or call the class on a return or the class might call the inst file (cf. https://github.com/mzechmeister/serval/blob/620156a7346af8ca6fc1e91dec8aba1522b019b0/src/read_spec.py#L91).

JanaKoehler1985 commented 1 year ago

You're sure you want to change everything to class? I mean, the class method is a nice thing and can make things more clear (if used in a good way). But normally you make this decision at the beginning before you start writing a code. Changing that on the present code can be a pain in the ass. And we have to make sure that everything is still working in the end.
You think it is worth the effort and time?