search

sbird / fake_spectra

A code for generating fake spectra from a cosmological simulation
MIT License
12 stars 13 forks source link

"res" and "spec_res" could have better names #12

Closed andreufont closed 6 years ago

andreufont commented 6 years ago

These are both arguments in the constructor of Spectra class, and they can be a bit confusing.

It would be great if "res" was renamed "pixel_width" or similar.

It got worse because until now @keirkwame was using his variable "spectrum_resolution" in lyman_alpha repo to feed "spec", and not "spec_res".

sbird commented 6 years ago

I agree that pixel_width would be a generally better name if starting from scratch, but for compatibility with existing scripts I'd rather not change it. The name is I think inherited from the earlier code by Jamie Bolton, so people who have used that will be expecting it to be called res.

I have updated and clarified the main comment on the Spectra class, can you let me know if that is easier to understand now?

andreufont commented 6 years ago

HI @sbird - Yes, the documentation is now clearer, although it says that spec_res is the FWHM and I believe it is the Gaussian rms. At least that is how it is used in the _window_function function. I would also specify the units (km/s).

I also noticed that until now the default was spec_res=8 km/s, what probably means that the optical depth rescaling was not correct. I'm not sure how much this would affect ongoing projects (emulators or the tests by @Chris-Pedersen), but it is probably worth re-running these.

sbird commented 6 years ago

@andreufont : Indeed! The optical depth rescaling was not correct, especially because we were setting the BOSS spectrograph value of spec_res = 69 in the emulator project. This was exactly the change for which I re-ran the emulator spectra the other day!

Documentation should be corrected when I get to somewhere that git push works.

Incidentally, the gaussian smoothing is applied to the flux, not the optical depth, can you confirm that this is correct?

andreufont commented 6 years ago

Yes, the smoothing should be applied to flux.

sbird commented 6 years ago

Cheers. Closing, with doc change push pending.