Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
andreufont opened this issue Apr 27, 2020 · 8 comments
Open

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

andreufont opened this issue Apr 27, 2020 · 8 comments

Comments

@andreufont
Copy link
Contributor

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.

dwave_out = 1.0

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

@sbailey
Copy link
Contributor

sbailey commented Apr 28, 2020

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
Copy link
Contributor

alxogm commented Jul 17, 2020

coming back to this issue. We fixed this for quickquasars mocks in #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 or desisim, 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
Copy link
Contributor

sbailey commented Jul 17, 2020

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
Copy link
Contributor

alxogm commented Jul 17, 2020

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
Copy link
Contributor Author

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

@alxogm
Copy link
Contributor

alxogm commented Nov 4, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

alxogm commented Nov 4, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants