[python/r] Enforce dataframe domain lower bound == 0#3300
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3300 +/- ##
==========================================
+ Coverage 85.37% 85.50% +0.12%
==========================================
Files 52 52
Lines 5499 5506 +7
==========================================
+ Hits 4695 4708 +13
+ Misses 804 798 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I'm confused, does the lowest value for |
|
@mojaveazure it means the domain needs to start at 0 at create time. You can still write 10, 20, 30 at write time. |
mojaveazure
left a comment
mojaveazure
left a comment
There was a problem hiding this comment.
LGTM from the R side, left one minor comment that can be ignored if need be. Please remember to bump the develop version and update the changelog when this gets shipped
|
@mojaveazure re
all I see is #3300 (comment) ... do you have another comment still pending? |
|
@mojaveazure thanks! @nguyenv any thoughts on the Python side? (I suspect not, I think this is a very basic thing, one in fact I thought we were already going -- but still I want to check with you.) |
| lower <- domain[["soma_joinid"]][1] | ||
| stopifnot("The lower bound for soma_joinid domain must be 0" = lower == 0) |
There was a problem hiding this comment.
Apparently, this never got submitted with my review 🤦 Is the order required or can we allow alternate ordering so long as we have a min and max? If ordering is required, we should check for that; otherwise, we should adjust this check to lower bound rather than first value
| lower <- domain[["soma_joinid"]][1] | |
| stopifnot("The lower bound for soma_joinid domain must be 0" = lower == 0) | |
| stopifnot("The lower bound for soma_joinid domain must be 0" = min(domain[["soma_joinid"]]) == 0) |
(@johnkerl feel free to ignore this if neither check is worth implementing)
There was a problem hiding this comment.
@mojaveazure the lower slot needs to be 0. Only that.
There was a problem hiding this comment.
They can't say (9,0); they have to say (0, 9).
There was a problem hiding this comment.
Then maybe also include a check that domain[["soma_joinid"]][2L] > domain[["soma_joinid"]][1L] (or >= if both are allowed to be 0)? (I'm assuming c(0L, -10L) is disallowed, but not sure)
|
Thanks @nguyenv ! |
johnkerl
force-pushed
the
kerl/sdf-sjid-lower-zero
branch
from
5f01962 to
b1f55f3
Compare
3 participants
Arose during discussion of #2407 / [sc-51048], but the defect here predates the new-shape work.
Update 2024-11-20: please see #3358 which reverts this.