desihub / desisim

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

Change default pixel width of DESI mocks to 0.8A #532

Open andreufont opened 4 years ago

andreufont commented 4 years ago

We recently found out that the Lyman alpha mocks had pixels of width 1A, instead of 0.8A.

We think this might be related to line 619-620 in simexp.py, that sets by default the output pixel width to 1A, when the user doesn't provide any value for dwave_out.

https://github.com/desihub/desisim/blob/3ae76e4c921f72b71ff7522462740e904136f428/py/desisim/simexp.py#L620

How confident are we that the final pixel width will be 0.8A? If we are confident, should we change this default value?

sbailey commented 4 years ago

I wouldn't be surprised if 0.8 changed again, but we might as well be consistent in the meantime (including the exact phasing of the 0.8 grid). I've added this to the 20.5 project to keep it on our radar for the next release. Contributions welcome. See desispec/bin/desi_proc lines 680-685 for current defaults (and yes, it is unfortunately hardcoded in a script; consider cleaning that up while implementing this so that desisim automatically uses whatever desispec is using as default).

alxogm commented 4 years ago

coming back to this issue. We fixed this for quickquasars mocks in desihub/desisim#533 . It seemed everything was right, but the balfinder is crashing for this mocks due to the following error

ERROR:coaddition.py:230:coadd_cameras: Cannot directly coadd the camera spectra because wavelength are not aligned, use --lin-step or --log10-step to resample to a common grid

It works well for a pixel width of 1 and 0.6, but not 0.8... The error is rised in #L225-L231 in desispec, and particularly fails for the 'z' band. I can, indeed, fix it by resampling the spectra with a linear step before making the coaddition, but I wanted to be sure this isn't really an issue in desispec ordesisim, at the time of creating the wavelength array... In the SV data we don't see this failure because the spectra is already coadded... @sbailey do you know if this is an issue maybe related to the more general change that should be done to use the pixel width in mocks in general, or is it somehow expected?

sbailey commented 4 years ago

The 3 cameras have overlapping wavelength coverage, and for coaddition to work (without re-sampling) it requires that the different grids are aligned, not just that they have the same step. e.g. OK:

wave_b = [..., 5759.2, 5760.0 , 5760.8, 5761.6, 5762.4]
wave_r =              [5760.0 , 5760.8, 5761.6, 5762.4, 5763.2, ...]

but not OK:

wave_b = [..., 5759.2, 5760.0 , 5760.8, 5761.6, 5762.4]
wave_r =              [5760.2 , 5761.0, 5761.8, 5762.6, 5763.4, ...]

Check your simulations to see that they have aligned wavelengths grids on the 3 cameras, not just that they all have 0.8 A steps.

FWIW, this is the grid used in Andes, though in the future we expect to expand that a bit more to get those last few photons off the edge of the CCDs:

wave_b = [3600. , 3600.8, 3601.6, ..., 5798.4, 5799.2, 5800. ]
wave_r = [5760. , 5760.8, 5761.6, ..., 7618.4, 7619.2, 7620. ]
wave_z = [7520. , 7520.8, 7521.6, ..., 9822.4, 9823.2, 9824. ]
alxogm commented 4 years ago

Check your simulations to see that they have aligned wavelengths grids on the 3 cameras, not just that they all have 0.8 A steps.

Yes, precisely the 'r' and 'z' bands are not aligned, and that rises the issue in #L225-L231. The modification in quickqusars was rather simple, just to propagate an argument to control the pixel width. So I guess the edges for the wavelength arrays are defined somewhere and we need to check they are aligned for the requested pixel width. I'll dig a bit more. Thanks @sbailey !

andreufont commented 2 years ago

Hi @alxogm - Was this fixed in quickquasars? If so, should we close the issue?

alxogm commented 2 years ago

Hi @andreufont, it was not fully solved since we changed the pixel width to 0.8A, but the overlapping wavelength range of the cameras is not aligned, so coaddition using desispec routine fails. I think it is until now that analysis on P1D mocks are facing this issue again, we can try to fix it.

andreufont commented 2 years ago

Hi @alxogm - I think we should change this, but this probably means increasing by one the "quickquasar version". I mean, these should probably be desi-3.0-4 mocks and so on, just like we changed from desi-1 to desi-2 when we changed the storing of the continuum and resolution matrices. Are there other minor changes that we wanted to do, so that we could coordinate them all and put them in the same version update?

alxogm commented 2 years ago

Yes, I think there are some small improvements that we can add to the same version code. I'll make a list in another issue, or PR, so we keep track of it.