-
Notifications
You must be signed in to change notification settings - Fork 12
Add rectangle class to math module #1815
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1815 +/- ##
==========================================
+ Coverage 99.04% 99.05% +0.01%
==========================================
Files 289 289
Lines 10986 11000 +14
==========================================
+ Hits 10881 10896 +15
+ Misses 105 104 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
This seems like something that scanspec achieves. Have you had a look at from scanspecs.regions import Rectangle for example?
|
Emm, no - I didn't, but for such simple thing I don't think it's necessary to rely on "yet another external python package" which can change with time |
DominicOram
left a comment
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.
Emm, no - I didn't, but for such simple thing I don't think it's necessary to rely on "yet another external python package" which can change with time
I disagree, scanspec is:
- Already in the bluesky ecosystem
- Actively maintained by people inside DLS
- Already imported by
dodal
If it doesn't fit the usecase that's fine but then can you link to a PR/issue explaining the usecase? I'm worried that this is a slippery slope to reinventing existing python libraries (which is particularly egregious if we're reinventing something written in the next office to ours)
|
With all respect - having a package in bluesky ecosystem does not guarantee it won't break downstream packages in a next release as proven countless times by dependent packages in bluesky ecosystem (ophyd-async - dodal - daq-server and so on) Also, I don't see what can be slippery in writing a simplest possible internal util Rectangle class. Sometimes reinventing simple things is not bad and gives more control and stability. With this being said - I don't think scanspec.region.Rectangle class fit into my simple purpose - I will write an issue explaining why. Thanks for looking at this PR! |
|
Added issue #1818 |
|
As a follow up on this discussion - regions.py was deleted from scanspec in this recent PR: ... |
DominicOram
left a comment
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.
With all respect - having a package in bluesky ecosystem does not guarantee it won't break downstream packages in a next release as proven countless times by dependent packages in bluesky ecosystem (ophyd-async - dodal - daq-server and so on)
Also, I don't see what can be slippery in writing a simplest possible internal util Rectangle class. Sometimes reinventing simple things is not bad and gives more control and stability.
With this being said - I don't think scanspec.region.Rectangle class fit into my simple purpose - I will write an issue explaining why. Thanks for looking at this PR!
Ok, you've convinced me. Thank you for adding the issue
Fixes #1818
IT might be handy to have it - specifically I will use it in the upcoming Apple Knot class which has rectangular exclusion zones.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}