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

Refactor top-level LSST metadetect #223

Merged
merged 12 commits into from
Mar 20, 2025

Conversation

arunkannawadi
Copy link
Contributor

This is a preview of the refactoring I mentioned in #220 . It currently refactors only the top-most level making run_metadetect, which is the typical entry point, a wrapper around a Task. The extensive coverage with unit tests should make us all feel confident going about such refactoring. The fact that the the unit tests pass without any changes also shows that the public APIs remain unchanged.

@arunkannawadi arunkannawadi changed the title U/arunkannawadi/task refactor 1 Refactor top-level LSST metadetect Oct 21, 2024
@arunkannawadi arunkannawadi force-pushed the u/arunkannawadi/task_refactor_1 branch from 739e57f to 313aac0 Compare October 31, 2024 16:42
Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Why were the ngmix upstream tests removed?

Ditto for the extra test skeips etc. All of that needs to be backed out before we can consider merging this.

@arunkannawadi arunkannawadi force-pushed the u/arunkannawadi/task_refactor_1 branch from c6cae7f to d058b72 Compare October 31, 2024 22:35
@arunkannawadi arunkannawadi marked this pull request as ready for review November 1, 2024 00:23
@arunkannawadi
Copy link
Contributor Author

Thanks to @ismael-mendoza for doing the big chunk of this work. We've tested this by also running the shear recovery tests in descwl-shear-sims (shear_test_descwl/test_shear_meas) and that seems to pass.

@arunkannawadi
Copy link
Contributor Author

@esheldon @beckermr - have you had a chance to go through this PR?

@esheldon
Copy link
Owner

This looks OK to me. I think we need to run it at scale to stress test it, and look at outputs for sanity checks.

@arunkannawadi
Copy link
Contributor Author

I am happy to run extensive tests but would need some handholding to get started.

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I have some comments/questions. Sorry for the delay!

Verified

This commit was signed with the committer’s verified signature.
arunkannawadi Arun Kannawadi

Verified

This commit was signed with the committer’s verified signature.
arunkannawadi Arun Kannawadi

Verified

This commit was signed with the committer’s verified signature.
arunkannawadi Arun Kannawadi

Verified

This commit was signed with the committer’s verified signature.
arunkannawadi Arun Kannawadi
Copy link
Contributor Author

@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.

I made changes/addressed the questions from previous review and I think this is ready for another round of review, Matt.

@arunkannawadi arunkannawadi requested a review from beckermr March 6, 2025 23:04

Verified

This commit was signed with the committer’s verified signature.
arunkannawadi Arun Kannawadi
rubin-env-10 now uses python 3.12 and is needed to run with the latest
stackvana.
This reverts commit fc0b66b.
Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

LGTM on the code! One more request.

Can we add some documentation somewhere or links to it to explain a bit about how the config works, how we can ensure the config is not changed, etc.?

We had a ton of discussion on this, but I forgot the results and it feels like we should write it down.

Verified

This commit was signed with the committer’s verified signature.
arunkannawadi Arun Kannawadi
@arunkannawadi arunkannawadi requested a review from beckermr March 20, 2025 15:57
@arunkannawadi
Copy link
Contributor Author

I've added a README to the lsst subpackage explaining some of this. Let me know what else you'd like to see.

Additionally, I'd like to include py3.12 to the matrix test (not necessarily in this PR though) but it looks like it couldn't find consistent set of dependencies. GitHub Actions log here.

@beckermr
Copy link
Collaborator

Looks like sep is not on numpy2 yet and the feedstock is falling behind. I am working on it now.

@arunkannawadi
Copy link
Contributor Author

Thank you! However, the LSST-specific tests, which is where I tried adding 3.12 should not require sep at all in the first place. It looks like it's trying to install everything in requirements.txt when only a subset of them are required for the lsst subpackage. sxdes is another one that's not required.

@beckermr
Copy link
Collaborator

Ah we can fix that as well by separating out the files. Are you adding 3.12 here or in another PR?

@arunkannawadi
Copy link
Contributor Author

I will add 3.12 in another PR.

@beckermr
Copy link
Collaborator

@arunkannawadi I can help there to separate out the requirements if needed!

@arunkannawadi
Copy link
Contributor Author

Alright, thanks. That'll be in a week or so after this week's stackvana is out. I don't have the permissions to merge the PR, so I'll leave that to you and Erin.

@beckermr
Copy link
Collaborator

@esheldon you ok to merge here?

@esheldon esheldon merged commit 3a48779 into esheldon:master Mar 20, 2025
6 checks passed
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.

None yet

4 participants