-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: master
Are you sure you want to change the base?
Conversation
evevkovacs
commented
Aug 30, 2019
@yymao I am not sure how to deal with the following travis issues: |
There was a problem hiding this 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?
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 | ||
""" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
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?