QSOGen Quasar SEDs for realistic colors and magnitudes for LSST Euclid and Roman bands#397
QSOGen Quasar SEDs for realistic colors and magnitudes for LSST Euclid and Roman bands#397timedilatesme wants to merge 42 commits intoLSST-strong-lensing:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #397 +/- ##
==========================================
+ Coverage 98.42% 98.50% +0.07%
==========================================
Files 99 102 +3
Lines 7511 7804 +293
==========================================
+ Hits 7393 7687 +294
+ Misses 118 117 -1
🚀 New features to boost your workflow:
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Hello @sibirrer , Finally the tests are complete for this PR and it is ready for review. I also add @Henry-Best-01 for a quick look especially at the quasar.py file where I have defined a new method to initialize the agn_class which was necessary as this class contains info about parameters necessary for microlensing calculations (w/in the update_microlensing_kwargs_source_morphology method) so I had to define it outside the light_curve method. @Henry-Best-01 : So currently the mean magnitudes are drawn from the QSOGen while variability is drawn from the Thin Disk Model currently implemented. I am working on incorporating your suggestions from last Thursday so that we effectively use observationally inferred temp. profile for the variability as well. I will try to fix that in another PR. Also the updated notebook now shows correct colors for the generated quasar population. |
|
Thank you @timedilatesme ! I will assign @Henry-Best-01 as reviewer (need to be added as triage in this repo first). |
Henry-Best-01
left a comment
Henry-Best-01
left a comment
There was a problem hiding this comment.
Overall great! I left some comments across various files, please let me know if any suggestions don't make sense.
| alpha_red: -0.5 | ||
| cosmology: !astropy.cosmology.default_cosmology.get [] | ||
| filters: [ | ||
| 'Roman-F062', 'Roman-F087', 'Roman-F106', 'Roman-F129', 'Roman-F158', 'Roman-F184', 'Roman-F146', 'Roman-F213', |
There was a problem hiding this comment.
Just wondering if you missed 'lsst2016-u'. Disregard this comment if it's not important.
There was a problem hiding this comment.
so by default in skypy yml files we've not been including u band info as it's only available for galaxies upto redshift of 4. After which skypy SEDs don't work. This is the same as other previously defined defined yml files as well.
However, if one wants to include u band info then that can be done by simply specifying it while instantiating the SkypyPipeline class, which does update this yml file. so I think it should be fine to miss u band by default
| magnitudes: $red.M | ||
| coefficients: $red.coeff | ||
| filter: bessell-B | ||
| mag_F062, mag_F087, mag_F106, mag_F129, mag_F158, mag_F184, mag_F146, mag_F213, mag_g, mag_r, mag_i, mag_z, mag_y: !skypy.galaxies.spectrum.kcorrect.apparent_magnitudes |
There was a problem hiding this comment.
Similar to above, just wondering if you're missing 'mag_u'
| cosmology: !astropy.cosmology.default_cosmology.get [] | ||
| filters: [ | ||
| 'Roman-F062', 'Roman-F087', 'Roman-F106', 'Roman-F129', 'Roman-F158', 'Roman-F184', 'Roman-F146', 'Roman-F213', | ||
| 'lsst2016-g', 'lsst2016-r', 'lsst2016-i', 'lsst2016-z', 'lsst2016-y'] |
There was a problem hiding this comment.
Just wondering if the 'lsst2023' filters should be used here to match up to date speclite filters. If these are required to query SkyPy, disregard since I am not an expert on using SkyPy.
There was a problem hiding this comment.
I think yes we should use those, I was just following what was already there for other yml files but i think it's better to update everything to 2023 conventions. perhaps in another PR.
| return [get_speclite_filtername(band) for band in bands] | ||
|
|
||
|
|
||
| ALL_SUPPORTED_BANDS = [ |
There was a problem hiding this comment.
Is ALL_SUPPORTED_BANDS used, or is this an artifact of keeping track of the bands? If unused, perhaps it should be placed in a comment at the top of the file for informational purposes. Disregard this comment if used elsewhere.
There was a problem hiding this comment.
Never mind, I found where you import it!
There was a problem hiding this comment.
In the future, perhaps we can convince Matthew to make his code pip installable and remove these files. Until then, no problem!
|
|
||
| # Use loose tolerance because emission lines are added ON TOP of the continuum | ||
| # which might shift the exact value at 3000A slightly | ||
| assert np.isclose(qs.flux[idx_3000], expected_f3000, rtol=0.2) |
There was a problem hiding this comment.
Is it worthwhile to test with a quasar spectrum where the emission lines are not added / extremely suppressed? You should be able to set base_params["scal_emline"] = 0 or 1e-8 to do this I believe.
Then the tolerance can be tightened up a little.
There was a problem hiding this comment.
souns great! added this test.
| base_params["gflag"] = True | ||
|
|
||
| # Wavelength array that does NOT cover 4000-5000A | ||
| bad_wav = np.linspace(1000, 3000, 500) |
There was a problem hiding this comment.
Can you add a test when the bad wavelength partially covers this range, and is only values larger than this range? (e.g. ranges (3000 - 4400) and (6000 - 10000) )
There was a problem hiding this comment.
added this test as well!
| redshifts=np.linspace(0.5, 1.0, 5), | ||
| sky_area=Quantity(0.05, unit="deg2"), | ||
| noise=True, | ||
| cosmo=FlatLambdaCDM(H0=70, Om0=0.3), |
There was a problem hiding this comment.
QSOGen assumes a cosmology of H0 = 71, Om0 = 0.27 which is hard coded. I think this should be tested consistently by changing these parameters to match.
There was a problem hiding this comment.
now QSOGen uses the same cosmology as QuasarRate!
| assert np.all(table["ps_mag_r"] > 0) | ||
|
|
||
| # Check that the magnitudes are distinct (g != r) | ||
| assert not np.allclose(table["ps_mag_g"], table["ps_mag_r"]) |
There was a problem hiding this comment.
Is there the possibility that an emission line + redshift combo rarely makes these magnitudes equal? I am not sure what the precision on this columns values are, but if you have precision of 1 or 2 decimal places (e.g. due to modeling mag_err), I imagine sometimes this test could fail.
There was a problem hiding this comment.
It could certainly be possible that for a few quasars it may turn out to be equal. But certainly it would not be equal for all of them. The assertion will be correct if at least one of them has these magnitudes different which might mean that this test would not fail for a broad redshift range? I am keeping absolute tolerance to be millimags and I think the mags should be not this close for all quasars.
There was a problem hiding this comment.
Ahh great, millimags will be fine! I sometimes mix up what np.all() and np.any() do for tables. I appreciate the clarification!
| # Typical quasar colors are between -1 and +2 roughly | ||
| assert np.all( | ||
| np.abs(g_i_color) < 5.0 | ||
| ), "Derived colors are unphysical, anchoring likely failed." |
There was a problem hiding this comment.
Can you test if this does fail at high redshift?
There was a problem hiding this comment.
I tried testing this for z upto 5.5 which is the max i could test for with current implementation (that uses Richards QLF), I did not see this failing upto those redshifts. Perhaps It'll fail after 7? Do you think then it might be worth to keep the failing test for high-z? Maybe not now but in future?
There was a problem hiding this comment.
I think it would be useful to define a mock file that has redshifts beyond 5.5 (something simple like integer redshifts 1 through 12 with some sort of mag _i associated with them, the exact values don't matter). I'm thinking in the future we'll have expanded catalogs similar to Richards' and a small test that the architecture will support them is nice.
|
Hi @Henry-Best-01 , Thank you so much for the review! I have tried addressing mostly all of your comments and here's a summary:
Please let me know if any of this needs further improvement! |
Henry-Best-01
left a comment
Henry-Best-01
left a comment
There was a problem hiding this comment.
Great work!
The only last addition I would request is a small test catalog (perhaps ~10 lines) that extends to z ~ 12. The values don't need to be realistic, but it would be good to see if all the code in place is able (or not!) to handle it. If this is difficult or time consuming, then I would say it's okay to not worry about it!
3 participants
Currently SLSim quasars are too blue. This PR aims to solve that by using empirical quasar SEDs and compute magnitudes while generating the quasar population with QuasarRate class. Here is a list of what's complete and what's not.