[python/r] Enforce dataframe domain lower bound == 0

[johnkerl]

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.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.50%. Comparing base (208c0ab) to head (b1f55f3).
Report is 1 commits behind head on main.

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     

Flag	Coverage Δ
python	85.50% <85.71%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
python_api	85.50% <85.71%> (+0.12%)	⬆️
libtiledbsoma	∅ <ø> (∅)

[mojaveazure]

I'm confused, does the lowest value for soma_joinid have to be 0 or does it have to be 0 or greater? IIUC setting soma_joinid to c(10, 20, 100) was valid, but the check added here seems to say otherwise

[johnkerl]

@mojaveazure it means the domain needs to start at 0 at create time.

You can still write 10, 20, 30 at write time.

[mojaveazure]

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

[johnkerl]

@mojaveazure re

left one minor comment that can be ignored if need be

all I see is #3300 (comment) ... do you have another comment still pending?

[johnkerl]

@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.)

[mojaveazure]

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

Suggested change

	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)

[johnkerl]

@mojaveazure the lower slot needs to be 0. Only that.

[johnkerl]

They can't say (9,0); they have to say (0, 9).

[mojaveazure]

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)

[johnkerl]

Good call, thanks @mojaveazure !

[johnkerl]

0f4f525
b7c72df

[nguyenv]

lgtm!

[johnkerl]

Thanks @nguyenv !