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

Ellipticity #81

Merged
merged 9 commits into from
Feb 24, 2018
Merged

Ellipticity #81

merged 9 commits into from
Feb 24, 2018

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.
Plots for 2 variants of this test are here:
https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-02-23_24
https://portal.nersc.gov/project/lsst/descqa/v2/?run=2018-02-23_23
Contributing code and 2 yaml files. First variant plots e distributions for varoiuos morphologies and compares to COSMOS data. Second variant plots default ellipticity distributions for catalog.
Just tested on buzzard. Throws an error that needs to be fixed, but can be done in parallel with other code improvements.

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.

I've pushed some changes (clean up the code to pass pylint checks, remove the binary tarball file, and rename the tests to ellipticity_dist and ellipticity_dist_COSMOS). See my changes in detail here.

Let me know if they look good. If so then we are ready to merge.

@yymao
Copy link
Member

yymao commented Feb 24, 2018

@evevkovacs BTW, are the validation datasets the raw data? I think we should just check in reduced data (like histograms).

Copy link
Contributor Author

@evevkovacs evevkovacs left a comment

Choose a reason for hiding this comment

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

looked good, but then I just saw your latest test run and the ellipticity distribution test was skipped for protoDC2, and that shouldn't have happened.

@evevkovacs
Copy link
Contributor Author

Hmm.. did I forget to commit the data? The files came from BenjaminJoachimi. I don't think they are too big. Maybe we can just use them for now and decide later?

@evevkovacs
Copy link
Contributor Author

We won't be able to change the binning if we just save the histograms

@yymao yymao merged commit 15a5fdc into master Feb 24, 2018
@yymao yymao deleted the ellipticity branch February 24, 2018 21:34
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.

None yet

2 participants