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

[ENH] Add compressed TSV files to the common principles #1749

Merged
merged 17 commits into from
Apr 2, 2024

Conversation

oesteban
Copy link
Collaborator

@oesteban oesteban commented Mar 25, 2024

Their description is hedged within the physiological recordings so upcasting them to the common principles seems important.

Their description is hedged within the physiological recordings so
upcasting them to the common principles seems important.
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.93%. Comparing base (bd08602) to head (cab6e9d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1749   +/-   ##
=======================================
  Coverage   87.93%   87.93%           
=======================================
  Files          16       16           
  Lines        1351     1351           
=======================================
  Hits         1188     1188           
  Misses        163      163           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

oesteban added a commit to oesteban/bids-specification that referenced this pull request Mar 25, 2024
src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
oesteban added a commit to oesteban/bids-specification that referenced this pull request Mar 25, 2024
@oesteban oesteban mentioned this pull request Mar 25, 2024
9 tasks
@Remi-Gau
Copy link
Collaborator

Slightly related to #644

@Remi-Gau
Copy link
Collaborator

I think that this reverts some of #1699, no?
Specifically the exception to the rule for tsv for motion data as motion.tsv are header less.

@oesteban
Copy link
Collaborator Author

oesteban commented Mar 25, 2024 via email

@oesteban
Copy link
Collaborator Author

I think that this reverts some of #1699, no? Specifically the exception to the rule for tsv for motion data as motion.tsv are header less.

Okay, I'm realizing a problem -- motion's tsv files ARE NOT compressed, is that right?

I'll push a commit improving some of my changes not to undo the motion exception.

@Remi-Gau
Copy link
Collaborator

I think that this reverts some of #1699, no? Specifically the exception to the rule for tsv for motion data as motion.tsv are header less.

Okay, I'm realizing a problem -- motion's tsv files ARE NOT compressed, is that right?

I'll push a commit improving some of my changes not to undo the motion exception.

Yup. Sorry. Pre coffee me was not clear.
This is an "inconsistency" we realized only recently .

@oesteban
Copy link
Collaborator Author

Okay, compatibilization with #1699 is done (c4ee8d6).

Slightly related to #644

I doubled down with 4eadad5. I am happy to see that changed if #644 leads the table somewhere else later on.

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
oesteban added a commit to oesteban/bids-specification that referenced this pull request Mar 25, 2024
oesteban added a commit to oesteban/bids-specification that referenced this pull request Mar 25, 2024
oesteban added a commit to oesteban/bids-specification that referenced this pull request Mar 25, 2024
src/common-principles.md Outdated Show resolved Hide resolved
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
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.

Okay, reading through, I think this is good overall. Some specific proposals.

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@sappelhoff
Copy link
Member

I'll try to think how to make it more clear.

Thanks, I'll mark the parts that I find a bit ambiguous.

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Copy link
Collaborator Author

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

@sappelhoff please have a look and maybe iterate over my suggestions to make the reading of the specs unequivocal regarding TSV vs TSVGZ

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
@oesteban
Copy link
Collaborator Author

@effigies @Remi-Gau @sappelhoff please check the changes to make unambiguous that .tsv and .tsv.gz are not interchangeable 48ffe9d

It feels too verbose to me, but I agree with @sappelhoff we need to avert any diverting readings of the specs. TBH, understanding that tsv.gz can be used instead of tsv at every instance felt a bit forced to me, but it's better to make the interpretation absolutely unambiguous.

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.

Thanks for the adjustments and the general proposal! I am happy to see this merged either with or without my proposed subheading change.

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 think this is a reasonably compact update that eliminates the possible misreading.

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
Please revert if these should not be replaced.
oesteban added a commit to oesteban/bids-specification that referenced this pull request Mar 27, 2024
src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@oesteban
Copy link
Collaborator Author

oesteban commented Apr 2, 2024

Considering that the minimum period for discussion has been safely satisfied, the very incremental nature of the proposal, the absence of major side effects on the specs, and the possibility of ironing around yet-to-be-identified wrinkles through the PR mechanism anytime in the future, I think I can go ahead and merge.

There are a few aspects to follow up from here:

@oesteban oesteban merged commit 5d087ff into bids-standard:master Apr 2, 2024
26 checks passed
@oesteban oesteban deleted the fix/large-tabular-files branch April 2, 2024 08:53
oesteban added a commit to oesteban/bids-specification that referenced this pull request Apr 2, 2024
oesteban added a commit to oesteban/bids-specification that referenced this pull request Apr 2, 2024
effigies added a commit that referenced this pull request Apr 11, 2024
#1750)

* fix: move ``_stim.<ext>`` specification within the task events module

Depends-on: #1749.

* Apply suggestions from code review

Co-authored-by: Remi Gau <remi_gau@hotmail.com>

* Update src/modality-specific-files/task-events.md

* Update src/modality-specific-files/physiological-recordings.md

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* enh: improve admonitions

* Apply suggestions from code review

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

* fix: markdown link with linebreak

* fix: missing literal closing backtick

* Apply suggestions from code review

Co-authored-by: Remi Gau <remi_gau@hotmail.com>

* fix: typo

* fix: consistency and ``[recording-<label>]`` soft removal

* enh: add synthetic example

* fix: defining links inside admonition

* Apply suggestions from code review

* Update src/modality-specific-files/task-events.md

---------

Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@sappelhoff sappelhoff changed the title ENH: Add compressed TSV files to the common principles [ENH] Add compressed TSV files to the common principles Jun 13, 2024
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