-
Notifications
You must be signed in to change notification settings - Fork 54
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
Pbc feature location fix #350
Pbc feature location fix #350
Conversation
…e it instead returns the 'low' value
…eeding, and to take the modulo of the pixel location to prevent out of bounds errors
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.x #350 +/- ##
==========================================
Coverage 55.75% 55.75%
==========================================
Files 15 15
Lines 3313 3316 +3
==========================================
+ Hits 1847 1849 +2
- Misses 1466 1467 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the fix @w-k-jones !
Just to clarify the change in the column seeding: did you do this because you encountered any issues with the previous solution or is it mainly that you think it makes more sense to choose the closest index rather than rounding down?
And: any idea what is going on with the RTD check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also happy with this pending RTD getting fixed. It seems likely to me that the issue is similar to what I encountered in #337, and we may need to do the same fix here.
So it turned out that the RTD issue was with the requirements, but that's fixed now. The column seeding change was because I think it makes more sense to round to the nearest location (and was how I expected the code to work before I looked into it). RTD is now building, so should be ok to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes also sense in my view with the column seeding! Please go ahead and merge :) great that you solved the RTD issue!
…segmentation due to calling int/float on a length 1 array
…es/tobac into PBC_feature_location_fix
I've just made a small update to segmentation to fix #348 at the same time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this, @w-k-jones .
Fixes out-of-bounds error in segmentation due to PBC features in #349
I also changed the location where 'column' seeding places the marker. Previously it took the floor of the array index, but now it goes to the nearest index. This may change the results of segmentation, so I can revert it if it is an issue