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

Pending changes requested on previous PR, from_catalog fix for Ohio mocks #580

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

HiramHerrera
Copy link
Contributor

This Pull request addresses various of the requested changes that were pending in #578. Most of the comments were addressed except for one:

Ah this is the seed. I recommend creating a member random engine rng_engine = np.random.default_rng(seed) and calling it to generate the random numbers you need. default_rng is recommended since numpy 1.17

I tested doing this and the resulting catalogs are different than the obtained with the currently implemented method. For the sake of reproducibility of the KP6 mocks I will leave it as is. Changing to a random engine will be left for future implementations.

Some additional changes in this PR:

  1. Fixed a bug causing quickquasars to assign the magnitudes and exposure times from the input catalog to the incorrect MOCKID. This happens if the sorting order of the MOCKIDs in the input catalog is different to the sorting order in the metadata of the raw transmission files. I checked that this does not affect neither the Y1 Saclay or LyaCoLoRe Mocks for two main reasons.
    • In both cases the MOCKID are sorted the same way in the input catalog and metadata. Therefore, the magnitudes and exposure times were correctly assigned.
    • For LyaCoLoRe and Saclay we don't care much about what target gets assigned which magnitude, since these are random and only depend on the target's redshift. This is more relevant in Ohio mocks where we want specific targets to have specific magnitudes and exposure times for direct comparison with observed data.
  2. I made it possible to use the gen_qso_catalog.py script to generate an input catalog for Y5 mocks that will follow the redshift-magnitude distribution of the DESI-Y1 Lya Sample.

@coveralls
Copy link

Coverage Status

coverage: 44.679% (-0.09%) from 44.765%
when pulling 3b5c3ff on HiramHerrera:y1mocks_v3
into d087e8a on desihub:main.

@p-slash
Copy link
Contributor

p-slash commented Mar 22, 2024

Looks good to me. Thanks!

@p-slash
Copy link
Contributor

p-slash commented Mar 25, 2024

I generated P1D mocks using this branch. Here are my observations:

  • The code correctly generates quasars at the given redshifts and FLUX_R values.
  • However, for quasars z>3.7, the IGM absorption reduces the FLUX_R value. This recalculated FLUX_R is less than the input FLUX_R.

image

  • This means the input FLUX_R values should be amplified accordingly. Alternatively, we can address this within quickquasars by amplifying FLUX_R values.

@p-slash
Copy link
Contributor

p-slash commented Mar 25, 2024

I tested amplifying the FLUX_R values using the mean flux of the Ohio mocks. The catalog is here /global/cfs/cdirs/desicollab/users/naimgk/ohio-p1d/iron_exptime_fluxr_catalog.fits. This is what I get after running quickquasars:
image
Not perfect, but better. One caveat is that Ohio mocks are not tuned for high redshift mean flux, so they have less than the literature values.

I also compared IVAR values in B arm for some random spectra. The agreement between observed spectra and mock spectra okay.
image

@p-slash
Copy link
Contributor

p-slash commented Apr 1, 2024

@HiramHerrera Please go ahead and merge it. Good work!

@HiramHerrera HiramHerrera merged commit 532bb80 into desihub:main Apr 2, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants