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

DM-45899: Write a Task to compute Ex for TEx #389

Merged
merged 5 commits into from
Oct 15, 2024
Merged

Conversation

PFLeget
Copy link
Contributor

@PFLeget PFLeget commented Sep 25, 2024

@PFLeget PFLeget force-pushed the tickets/DM-45899 branch 3 times, most recently from 611250a to 66885c6 Compare September 27, 2024 14:52
@PFLeget PFLeget marked this pull request as ready for review September 30, 2024 14:21
@arunkannawadi arunkannawadi self-requested a review October 4, 2024 20:49
Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

There's actually a guide on how to transfer code between packages: https://developer.lsst.io/stack/transferring-code.html#transferring-code-between-packages

Although the history is minimal, it'd be a good practice to follow this procedure.

python/lsst/meas/algorithms/__init__.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
tests/test_computeExPsf.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
@PFLeget PFLeget force-pushed the tickets/DM-45899 branch 3 times, most recently from 994ec6c to 19db5fd Compare October 7, 2024 21:43
Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

The transfer branch should move the code as it was. Suggested doc changes and updating the default values to correspond to treecorr v4.3 should happen after the transfer branch is merged to the ticket branch.

python/lsst/meas/algorithms/treecorrUtils.py Show resolved Hide resolved
python/lsst/meas/algorithms/treecorrUtils.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/treecorrUtils.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

Are the default values in TreecorrConfig suitable for the metric calculation? I don't quite remember what binning values we settled on, and what the default ones would be. ComputeExPsfConfig should probably have a setDefaults method that would set it to the nbins, min_sep, max_sep and units values appropriately.

python/lsst/meas/algorithms/computeExPsf.py Outdated Show resolved Hide resolved
python/lsst/meas/algorithms/computeExPsf.py Show resolved Hide resolved
dtype=TreecorrConfig,
doc="treecorr config.",
)

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this needs an explicit validate method that calls self.treecorr.validate. Because TreecorrConfig is not a ConfigClass of a sub task, I am fairly certain that it will never be validated.

Copy link
Member

Choose a reason for hiding this comment

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

There also needs to be a setDefaults method here that sets the min_sep, max_sep and either nbins or bin_size as appropriate here corresponding to the requirements. @jmeyers314 - what was the binning we settled on finally for TEx computations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the TEx computations and default parameters, I chose to put the default parameters here instead.

I have the feeling this is a better place, but I can be wrong.

Also, I followed what is written in this doc for the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the validate, I tried in the unit test and putting wrong value in ComputeExPsfConfig (min_sep>max_sep) for example and it triggered the validate from TreecorrConfig. Do I need to do more than that?

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be best to put it here, given that i) we might want to run this dynamically outside of computeSummaryStats and ii) it makes sense for this class to have its own defaults for treecorr rather than computeSummaryStats setting them.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

Same comment as in analysis_tools PR - start the commit messages with upper case. Good to go after you set the defaults here in setDefaults.

@PFLeget PFLeget merged commit 0977469 into main Oct 15, 2024
2 checks passed
@PFLeget PFLeget deleted the tickets/DM-45899 branch October 15, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants