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

AGN number-density tests #178

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

AGN number-density tests #178

wants to merge 16 commits into from

Conversation

evevkovacs
Copy link
Contributor

If you are submitting a PR for a validation test (either adding a new one or updating an existing one), please post a link to the DESCQA web interface for the test you are submitting.
New agn number-density test. Sample test results are here

@evevkovacs evevkovacs requested a review from yymao August 30, 2019 19:04
@evevkovacs
Copy link
Contributor Author

@yymao I am not sure how to deal with the following travis issues:
(I tried adding # pylint: disable=too-many-instance-attributes to the code but it didn't help.)
descqa/AGN_NumberDensities.py:40:0: R0902: Too many instance attributes (24/7) (too-many-instance-attributes)
descqa/AGN_NumberDensities.py:44:4: W0102: Dangerous default value [] as argument (dangerous-default-value)
descqa/AGN_NumberDensities.py:44:4: R0913: Too many arguments (6/5) (too-many-arguments)
descqa/AGN_NumberDensities.py:143:4: C0112: Empty method docstring (empty-docstring)
descqa/AGN_NumberDensities.py:143:4: R0914: Too many local variables (44/15) (too-many-locals)
descqa/AGN_NumberDensities.py:143:4: R0912: Too many branches (13/12) (too-many-branches)
descqa/AGN_NumberDensities.py:143:4: R0915: Too many statements (56/50) (too-many-statements)
Thanks.

Copy link
Member

@yymao yymao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evevkovacs, thanks for the PR. The code is long so it may take me sometime to review this. I've relied to your question inline. In the meantime, can you assign someone to do a scientific review?

descqa/AGN_NumberDensities.py Outdated Show resolved Hide resolved
Copy link

@jiwoncpark jiwoncpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @evevkovacs, I had a few questions on the test design and output plots. Could you please catch me up on these points? There are also some minor comments on documentation.

z_lim: [0.3, 2.2]
observation: 'SDSS'
validation_range: [15.5, 19.]
agn_flux_fraction: 0.5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What motivated the minimum flux fraction of 0.5 here (and also in SDSSi_frac.5)? Is this supposed to be a comparison with the duty cycle model?

@@ -0,0 +1,105 @@
# RICHARDS ET AL. 2006_AJ_131_2766

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the QLF validation using Table 6 also planned? Maybe it'll go in a different PR? Please let me know if I'm missing something!


observation : string
string indicating which obsrvational data to use for validating
"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help to add the agn_flux_fraction to the parameter docstring!

band : string
photometric band

rest_frame_band : string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a description for the purpose of this argument, e.g. it's only used to query the K-corrected (z=0) magnitudes for applying the Mag_lim selection?

@@ -0,0 +1,25 @@
# RICHARDS ET AL. 2006_AJ_131_2766

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay if the extension of this file goes like .original? It does seem like it's not used...

@@ -0,0 +1,12 @@
subclass_name: AGN_NumberDensities.AGN_NumberDensity
band: g

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the number density agreement for the three SDSS_g* tests so poor? I'm looking at plots like
Nagn_vs_mag_g for the SDSS_g test on the cosmoDC2_v1.1.4_small_combined_agn catalog. I'll try running it for a bigger catalog; maybe the small one is missing the brightest quasars? Also, is there a list of catalogs this test is meant to run for and does DESCQA let you specify it anywhere?

@yymao yymao linked an issue Dec 2, 2020 that may be closed by this pull request
@yymao yymao changed the title Initial commit for AGN number-density tests AGN number-density tests Dec 2, 2020
@yymao yymao assigned evevkovacs and unassigned jiwoncpark Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add reader and tests for strong lensing catalogs
3 participants