Skip to content
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

[FIX] Revert index (entity format) definition to be a non-negative number, permitting zero #1482

Merged
merged 2 commits into from
May 8, 2023

Conversation

TheChymera
Copy link
Collaborator

@TheChymera TheChymera commented May 1, 2023

In continuation of previous discussions, including #535

A bit of theoretical background:

  • An index simply labels elements of another set. There's no particular reason for zero to not be an index, in fact it might be the most used one :)
  • The confusion zero sometimes causes is when it is used as an ordinal, since it leads to an asymmetric break in the number line by virtue of being its own inverse additive. Notably there is no year 0AD (or BC). Mathematically this can be considered an atavism, 0th is a valid ordinal, the problem with having a year 0 is that it causes asymmetry since distances between years would need to be calculated based on their start/end regardless of their position, and not based on their start if BC and end if AD.

More practically, there are a few datasets which include 0 values of an index:

and supporting this doesn't break or encumber anything (if anything it makes the regex matching marginally shorter).

If we were to explicitly exclude zero, I think ordinal would be a better notion, since zero is clearly what people think of as a valid index when reading the word index — though as explained above, even that is a bit arbitrary. The zeroth (nought) week is a thing at some colleges.

@effigies @yarikoptic

@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3d00184) 87.88% compared to head (86e06a5) 87.88%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1482   +/-   ##
=======================================
  Coverage   87.88%   87.88%           
=======================================
  Files          14       14           
  Lines        1279     1279           
=======================================
  Hits         1124     1124           
  Misses        155      155           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve in principle. A wording suggestion and a fix for the regular expression.

src/schema/objects/formats.yaml Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator

effigies commented May 1, 2023

Some additional context: #590 further used the non-negative definition. #885 converted to positive integer without any discussion that I noticed. Possibly I missed something though.

@effigies effigies changed the title Zero can be an index [FIX] Revert index (entity format) definition to be a non-negative number, permitting zero May 1, 2023
Co-authored-by: Christopher J. Markiewicz <effigies@gmail.com>
@TheChymera
Copy link
Collaborator Author

@effigies non-negative I think is more accurate. Zero is commonly unsigned, so we can have very esoteric discussions on the issues page about whether or not it's positive :3

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, non-negative is what we need, I think.

@effigies
Copy link
Collaborator

effigies commented May 2, 2023

Yes, it should be non-negative. I was pointing to the PR where it was changed to positive, so people could evaluate this fix.

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good with me.

Copy link
Collaborator

@bendhouseart bendhouseart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@TheChymera
Copy link
Collaborator Author

can we merge this to make sure it's in 1.9.0 ? :)

@effigies
Copy link
Collaborator

effigies commented May 8, 2023

Yes. We just have rules for changes to the spec to give people adequate time to comment.

@effigies effigies merged commit 5a765c4 into bids-standard:master May 8, 2023
@effigies effigies added this to the 1.9.0 milestone May 8, 2023
@TheChymera TheChymera deleted the index_format branch May 10, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants