-
Notifications
You must be signed in to change notification settings - Fork 85
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
Update SpatialSeries to have bounds field #1869
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1869 +/- ##
=======================================
Coverage 92.01% 92.02%
=======================================
Files 27 27
Lines 2619 2620 +1
Branches 685 685
=======================================
+ Hits 2410 2411 +1
Misses 139 139
Partials 70 70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Review Notes: We talked about the idea of validating data according to the bounds (if provided); however, we decided to not validate due to performance. As result, the only test added is to ensure the bounds field is mapped correctly to the spec. |
Sorry for the late review of this @mavaylon1 . Could you add a roundtrip test with the new field? |
Done. Sorry for the delay. |
Co-authored-by: Ryan Ly <[email protected]>
Co-authored-by: Ryan Ly <[email protected]>
Co-authored-by: Ryan Ly <[email protected]>
Motivation
What was the reasoning behind this change? Please explain the changes briefly.
Add test to SpatialSeries for bounds to make sure everything passes.
How to test the behavior?
Checklist
flake8
from the source directory.