-
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
shear test #54
shear test #54
Conversation
@patricialarsen thank you for submitting this PR! This is great progress and what needs to be done the next is to migrate the content in your notebook into the DESCQA framework. Basically you'll need to copy the content in cells Once you are done, you can then try running your test (follow instruction here) on NERSC and see your test appear on the web interface! Please don't hesitate to contact me if you encounter any issue and/or need more detail instruction. Thanks again! This is very nice! |
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.
See the inline comments. I haven't done a full review but start with ones that breaks the test.
Please also remove the jupyter notebook.
descqa/shear_test.py
Outdated
|
||
|
||
import sys | ||
sys.path.insert(0, '/global/common/software/lsst/common/miniconda/py3-4.2.12/lib/python3.6/site-packages') |
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.
remove both lines with sys.path.insert
(this will be taken care by the framework)
descqa/shear_test.py
Outdated
import yaml | ||
import numpy as np | ||
import healpy as hp | ||
from descqa.plotting import plt |
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.
remove this line as it's already imported above
descqa/shear_test.py
Outdated
import healpy as hp | ||
from descqa.plotting import plt | ||
|
||
from descqa import BaseValidationTest, TestResult |
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.
remove this line as it's already imported above
descqa/shear_test.py
Outdated
#from __future__ import unicode_literals, absolute_import, division | ||
import os | ||
import numpy as np | ||
from .base import BaseValidationTest, TestResult |
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.
reorder all imports --- (1) built-in modules, (2) installed modules, (3) this module (those start with from .
)
descqa/shear_test.py
Outdated
ax_this.plot(data, label=catalog_name) | ||
|
||
#self.post_process_plot(ax) | ||
#fig.savefig(os.path.join(output_dir, 'plot.png')) |
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.
uncomment these so it'll save the figure to be viewed on the web interface
sorry, I accidentally clicked approve... |
descqa/shear_test.py
Outdated
|
||
|
||
from GCR import GCRQuery | ||
import GCRCatalogs |
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.
remove this line as this will be taken care by the framework
Thanks @yymao for the notes - should I be testing this on the forked repository with: |
@patricialarsen yes. you can run the script in your cloned forked repo. You may want to use |
@yymao everything is working now, the reason this failed the build is because in the run_master script it uses "/global/common/cori/contrib/lsst/apps/anaconda/py3-envs/DESCQA/bin/python" when the camb and treecorr modules are located in I didn't want to make changes to the run_master script without first confirming with you, but when I swap the python environment it works and uploads the results to the website as expected. |
@patricialarsen thanks! I'll review the code soon. The python env in |
…r_corr Necessary as updates have been made to master branch that update python directory in run_master.sh file
Here's a preview of the test within the framework. It's for one bin around redshift 0.5-0.6 for protoDC2. Just the theory v simulations plots for now (x-axis is separation in arcminutes and y-axis is real-space correlation function), there's still no pass-fail criteria. https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-01-05_48&test=shear&catalog=protoDC2 |
Hi @patricialarsen - I looked at the test preview and have a few questions and comments:
|
Hi @rmandelb, Thanks for your feedback. Yes, these should be xi_plus and xi_minus. Sorry for the labelling confusion. And thanks for your suggestions on the plot - I'll incorporate those in the next version. For the extent of this test, it really depends how much testing we want on the catalogues at this verification stage. I can see it either being an initial validation test to make sure the catalogue is roughly okay (i.e. shears don't have any major bugs), or a test to see if the required levels of accuracy of the shear correlation functions are met. The current narrow redshift bin is just for testing and as the limits are input parameters in the .yaml file it's straightforward to broaden these or to use multiple bins. After I've determined some pass/fail criterion, tidied up the output and fixed the current version conflicts, it would be easy enough to make this incorporate:
|
@patricialarsen Thanks very much. I have one small cosmetic comment. It is difficult to distinguish the blue and green colors on the w+/xi+ plots. Perhaps a darker value or a different choice for one of the colors would help. |
@patricialarsen Just checking in --- what's the current status of this test? Is this ready to be reviewed? |
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.
Large chunks of commented-out code should be removed.
Line 20 to 29 --- these settings have cosmology dependence, shouldn't they be set by the cosmology of the catalog?
Yes, the cosmology ideally should be set to that of the catalog. I can update that assuming there's some way to query each catalog's cosmology? In the meantime since it's currently a rough check I think it's still useful to compare to some realistic cosmology. I'll remove commented out code - but will wait until you're done with changes so we don't conflict. |
I think this PR is ready to be merged. Before merging I'll run the latest version on NERSC again to make sure there's no issue. |
Example output of current version: https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-02-23_3 |
…into shear_corr It says I need this
Shear-Shear correlation function comparison between theory code using pyCAMB and simulation using TreeCorr. This does not yet output a test result or plot and there is some repeated code, but it computes both theory and simulation correlation functions within the DESCQA framework.