Light quickquasars #552
Light quickquasars #552
Conversation
- fixed bug in saving continuum
Changed flag to save resolution matrix
|
A full run was successful. Comparison of two runs with the same options: @andreufont I think this is ready for review. |
There was a problem hiding this comment.
@alxogm - these changes look great, and the improvement in disk space is amazing. I only added a couple of minor comments asking for documentation.
py/desisim/scripts/quickspectra.py
Outdated
| use_poisson : if False, do not use numpy.random.poisson to simulate the Poisson noise. This is useful to get reproducible random realizations. | ||
| use_poisson : if False, do not use numpy.random.poisson to simulate the Poisson noise. This is useful to get reproducible random | ||
| realizations. | ||
| save_resolution : if True it will save the Resolution matrix for each spectra. False is useful for mocks to save diskspace. |
There was a problem hiding this comment.
Maybe add that if save_resolution=False, this function returns a resolution matrix.
|
There will be minor changes to picca required for those data. The resolution HDUs are still there if I got the change right, so reading them should not be an issue. There is a minor issue about reading for Pk1d, it's a simple fix, but I'm not completely sure right now if you're storing it in 2d (ndiag,npix) or 3d (1,ndiag,npix) format. And is the new mode encoded somewhere in the headers? Depending on that I can fix the picca part of it easily... |
|
Thanks @andreufont, I think I have addressed your comments, and @Waelthus has access to the mock for adding a fix to picca. some tests are not passing, but I think is due to some warnings not related to this PR. |
|
The changes look great, happy for me to merge the PR. Not sure whether we should worry about the failed Travis tests though... |
I tried restarting the tests a couple of times, but they fail and I see only warnings, unrelated to this PR. @sbailey or @weaverba137 Could you confirm is ok to merge? Thanks! |
Hi @sbailey, maybe you missed this, can we merge even though some tests unrelated to this PR are not passing? |
|
Thanks for pinging me again. Yes it's ok to merge despite the "suspiciously low XYZ flux..." unrelated errors. |
We identified the need to have a lighter version of quickquasars mocks, in order to produce several realizations of them. We are reducing the file size by not saving the resolution matrix for each quasar in a pixel, since it is actually the same one. Instead we save only one resolution matrix once in the truth files.
We also identified we were saving the continuum with a very fine wavelength bin size, so we changed it from 0.2 to 2.
These changes allows to reduce the files for one pixel as follows:
Original
498M 0.4-1/spectra.fits
257M 0.4-1/truth-16-0.fits
596K 0.4-1/zbest-16-0.fits
This branch
102M 0.4-light1/spectra.fits
27M 0.4-light1/truth-16-0.fits
596K 0.4-light1/zbest-16-0.fits
One we check a full run proceeds without errors, I'll change it to ready for review. @jvstermer it would be good if you can test that the the wavelength binsize for the continuum is good enough.
@moonlovist it would be very useful if you can test this branch too. Thanks for your help identifying where to improve.