search

desihub / desisim

DESI simulations
BSD 3-Clause "New" or "Revised" License
16 stars 22 forks source link

Light quickquasars #552

Closed alxogm closed 3 years ago

alxogm commented 3 years ago

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.

alxogm commented 3 years ago

A full run was successful. Comparison of two runs with the same options: 770G /global/cfs/projectdirs/desi/mocks/lya_forest/develop/london/qq_desi/v9.0/v9.0.1/desi-1.4-4/spectra-16/ 133G /global/cfs/projectdirs/desi/users/alxogm/desi/mocks/qq_cheksize_full/spectra-16

@andreufont I think this is ready for review.

Waelthus commented 3 years ago

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... Can't test at the moment due to not having permission for the new mocks.

alxogm commented 3 years ago

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.

andreufont commented 3 years ago

The changes look great, happy for me to merge the PR.

Not sure whether we should worry about the failed Travis tests though...

alxogm commented 3 years ago

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!

alxogm commented 3 years ago

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?

sbailey commented 3 years ago

Thanks for pinging me again. Yes it's ok to merge despite the "suspiciously low XYZ flux..." unrelated errors.