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

Fix hsm assert when step or stride is < 0 #1186

Merged
merged 3 commits into from
Oct 21, 2022
Merged

Fix hsm assert when step or stride is < 0 #1186

merged 3 commits into from
Oct 21, 2022

Conversation

rmjarvis
Copy link
Member

The asserts in the hsm code are not correct when either step or stride is negative. For now, just skip these checks when either step or stride is negative. But possibly down the line we might want to make this check more rigorous to include the case of negative steps or strides.

@jmeyers314
Copy link
Member

Thanks, this looks good to me.

FWIW, I think rigorous here probably just means:

_minptr = _data + min(_step*(_ncol-1), 0) + min(_stride*(_nrow-1), 0);
_maxptr = _data + max(_step*(_ncol-1), 0) + max(_stride*(_nrow-1), 0) + 1;

and then return _minptr < p && p < maxptr;.

That doesn't seem too bad to give a go at now, but I admit I didn't try to look for all the possible ways _maxptr currently gets set.

@rmjarvis
Copy link
Member Author

I think I'll leave the rigorous version for later if it ever seems like it would be useful. Certainly adding another variable to the class now would cause ABI incompatibility, so we shouldn't knowingly do that on a bugfix release.

@rmjarvis rmjarvis merged commit bb85d07 into releases/2.4 Oct 21, 2022
@rmjarvis rmjarvis deleted the #1185 branch October 21, 2022 18:05
@rmjarvis rmjarvis added bug report Bug report hsm Related to the galsim.hsm adaptive moments tools labels Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Bug report hsm Related to the galsim.hsm adaptive moments tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HSM fails on Image constructed from flipped numpy array
2 participants