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.

Sign up for GitHub

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

Jump to bottom

DM-41489: Reimplement C++ HSM shear measurement algorithms in Python #47

Merged
merged 3 commits into from Dec 14, 2023
Merged

DM-41489: Reimplement C++ HSM shear measurement algorithms in Python #47

merged 3 commits into from Dec 14, 2023

Conversation

enourbakhsh
Copy link
Contributor

No description provided.

@enourbakhsh enourbakhsh force-pushed the tickets/DM-41489 branch 5 times, most recently from 2093017 to 5e3ad4e Compare December 1, 2023 08:25
arunkannawadi
arunkannawadi previously approved these changes Dec 1, 2023
View reviewed changes
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
@arunkannawadi arunkannawadi dismissed their stale review December 1, 2023 16:10

Hit Approved too soon

@enourbakhsh enourbakhsh force-pushed the tickets/DM-41489 branch 3 times, most recently from 994b079 to 7d0875f Compare December 3, 2023 22:32
@arunkannawadi
Copy link
Member

There's a whole bunch of code deletion that is pending. The folders include/, lib/ and src/ should be deleted to remove old code. ups/ should have only the .table file and the lines that refer to the lib folder should also be removed. The SConscript file should be updated to

scripts.BasicSConstruct("meas_extensions_shapeHSM", disableCc=True, noCfgFile=True)

jmeyers314
jmeyers314 requested changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

Looks great @enourbakhsh ! I left you a few questions/comments.

python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Show resolved Hide resolved
@arunkannawadi
Copy link
Member

You can already follow along the changes being made in #48 and at least use galsim._Image and galsim._Bounds instances.

enourbakhsh
enourbakhsh commented Dec 11, 2023
View reviewed changes
tests/test_hsm.py Outdated Show resolved Hide resolved
@enourbakhsh enourbakhsh force-pushed the tickets/DM-41489 branch 5 times, most recently from e3ce201 to 05d61f1 Compare December 12, 2023 21:59
arunkannawadi
arunkannawadi reviewed Dec 12, 2023
View reviewed changes
README.rst Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
python/lsst/meas/extensions/shapeHSM/_hsm_shape.py Outdated Show resolved Hide resolved
@enourbakhsh enourbakhsh force-pushed the tickets/DM-41489 branch 7 times, most recently from 71a2585 to 743ae42 Compare December 13, 2023 01:58
@enourbakhsh enourbakhsh force-pushed the tickets/DM-41489 branch 4 times, most recently from 61bfffc to 4714519 Compare December 13, 2023 04:23
jmeyers314
jmeyers314 reviewed Dec 13, 2023
View reviewed changes
README.rst Outdated Show resolved Hide resolved
jmeyers314
jmeyers314 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@jmeyers314 jmeyers314 left a comment

Choose a reason for hiding this comment

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

LGTM! Left you a few small items.

@enourbakhsh enourbakhsh merged commit ce06433 into main Dec 14, 2023
2 checks passed
@enourbakhsh enourbakhsh deleted the tickets/DM-41489 branch December 14, 2023 05:04
@mwittgen mwittgen mentioned this pull request Dec 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmeyers314 jmeyers314 jmeyers314 approved these changes

@arunkannawadi arunkannawadi arunkannawadi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@enourbakhsh @arunkannawadi @jmeyers314