Fix dvbin - Githubissues

sbird / fake_spectra

A code for generating fake spectra from a cosmological simulation

MIT License

12 stars 13 forks source link

Fix dvbin #48

Closed andreufont closed 4 years ago

andreufont commented 4 years ago

Addresses issue #47 , by changing the way we set self.nbins and self.dvbin in Spectra class.

To be decided:

Because of possible FFTs and similar, should we force self.nbins to be an even integer? I'm guessing maybe not, if you care about this you can choose the pixel width carefully yourself?
How noisy should we be if someone asks for a particular pixel width, but load spectra from savefile that has a different pixel width? The problem is that the option "res" is by default set to 1 (not None), so it is not easy to do the test, otherwise I'd make the code crash if there is a mismatch. For now I just print a warning message, and carry on with the code ignoring "res".

sbird commented 4 years ago

Thanks andreu! That is definitely a positive change. I think we don't need nbins to be an even integer.

It was perhaps a mistake to set res to 1, not None... would changing that default cause problems for you? I think for me it would be ok because I have always specified it manually.

andreufont commented 4 years ago

Happy to change the default to None, we always specify it as well.

I will make this change and update the pull request then.

sbird commented 4 years ago

Thanks! Let's go for it. If it causes problems they are fixable :-)

andreufont commented 4 years ago

Ok, I've tracked down all the "spectra classes" that had a default value for pixel width (res), and set that to None.

If you are reloading from the snapshot, and you do not specify a pixel width (res), the code crashes.

If you are loading from savefile, and you specified a pixel width (res), the code crashes if that value does not agree with the value read from savefile.

I'm not sure I've tested all possible ways this could go wrong, but it looks pretty safe to me.

sbird commented 4 years ago

Yes let's risk it. Thanks Andreu!

andreufont commented 4 years ago

Hi @sbird - I found a case where the new setup is a bit annoying. In Keir's lyman-alpha repo, that I also use in a project with the Sherwood simulations, the code choses the savefile filename using a combination of meta data, including res_kms. Some stored files are generated with res=5, others with res=10, and the filenames are 10.hdf5 and 5.hdf5.

To chose between one and the other you specify res=5 or res=10 in runtime, and it is nice that fake_spectra now does a check to make sure that the spectra loaded indeed has this resolution, but I set a relative tolerance of 1e-4, and I think this was a mistake.

If you don't mind, I'll set a 1% tolerance in the input value of spectral resolution. This would be enough for all practical matters, while keeping backward compatibility for some cases like the one I mentioned above that is now crashing. By the way, 0.1% would also be enough, but I think 1% is a safer call.

This would be a 1-character PR...