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-30163: Replace asserts by exceptions, cleanup calcmom #208

Merged
merged 2 commits into from Apr 13, 2022
Merged

DM-30163: Replace asserts by exceptions, cleanup calcmom #208

merged 2 commits into from Apr 13, 2022

Conversation

mwittgen
Copy link
Contributor

@mwittgen mwittgen commented Apr 4, 2022

No description provided.

arunkannawadi
arunkannawadi reviewed Apr 4, 2022
View reviewed changes
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.

Left a comment on Jira about how the exception should ideally be an InvalidParameterError and not a RuntimeError. Also, the commit message must start with a capitalized letter.

src/SdssShape.cc Outdated Show resolved Hide resolved
@mwittgen mwittgen changed the title DM-30163: replace asserts by exceptions or returned error flags DM-30163: Replace asserts by exceptions or returned error flags Apr 4, 2022
@mwittgen mwittgen force-pushed the tickets/DM-30163 branch 3 times, most recently from a9e0e93 to 1cf3ea0 Compare April 5, 2022 16:43
@mwittgen mwittgen changed the title DM-30163: Replace asserts by exceptions or returned error flags DM-30163: Replace asserts by exceptions, cleanup calcium Apr 5, 2022
arunkannawadi
arunkannawadi approved these changes Apr 5, 2022
View reviewed changes
Copy link
Member

@arunkannawadi arunkannawadi left a comment •
edited

Choose a reason for hiding this comment

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

I can confirm that the code that used to crash is now exiting gracefully with a useful error message. The commit has to split up into a clean up commit and the one that prevents the crash. My earlier comment about the capitalization was for the commit message, not the PR title. See https://developer.lsst.io/work/flow.html#appendix-commit-message-best-practices

src/SdssShape.cc
@@ -560,8 +574,9 @@ bool getAdaptiveMoments(ImageT const &mimage, double bkgd, double xcen, double y
*/
if (shape->flags[SdssShapeAlgorithm::UNWEIGHTED.number]) {
w11 = w22 = w12 = 0;
if (calcmom<false>(image, xcen, ycen, bbox, bkgd, interpflag, w11, w12, w22, &I0, &sum, &sumx, &sumy,
&sumxx, &sumxy, &sumyy, NULL, negative) < 0 ||
double ignored;
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this is ignored in place of sums4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the old code NULL was passed. One could rename it to sums4, but this is never used.
The old code used pointers to pass optional parameters C-style.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, may be still keep the variable name to psums4 or sums4 but just add a comment saying it is ignored? It'll prevent looking up the definition in the future to see what that NULL corresponds to.

src/SdssShape.cc
@@ -190,34 +190,23 @@ geom::Box2I computeAdaptiveMomentsBBox(geom::Box2I const &bbox, // full ima
/*
* Calculate weighted moments of an object up to 2nd order
*/
template <bool instFluxOnly, typename ImageT>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the option to compute instFluxOnly removed? It may not be used regularly, but it seems like a perfectly good option to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calcmon is overloaded now instFluxOnly instantiated two different versions of the functions with C style pointers.One could rename calcmon to make this explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. Makes sense. I'm happy as long as there's an option to calculate only the instFlux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the old code was C translated into bad C++.

@arunkannawadi arunkannawadi changed the title DM-30163: Replace asserts by exceptions, cleanup calcium DM-30163: Replace asserts by exceptions, cleanup calcmom Apr 7, 2022
@mwittgen mwittgen merged commit 45da3de into main Apr 13, 2022
@mwittgen mwittgen deleted the tickets/DM-30163 branch April 13, 2022 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunkannawadi arunkannawadi arunkannawadi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mwittgen @arunkannawadi