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

Remove accepted and rejected time series from the standard file outputs and change denoised time series name #1006

Closed
wants to merge 10 commits into from

Conversation

martaarbizu
Copy link
Contributor

@martaarbizu martaarbizu commented Nov 22, 2023

Closes #819 .

Changes proposed in this pull request:

  • Remove accepted and rejected time series from the standard file outputs
  • Change the name of optcomDenoised to denoised

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.55%. Comparing base (1c3f93e) to head (6bf777e).
Report is 63 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   89.54%   89.55%   +0.01%     
==========================================
  Files          26       26              
  Lines        3395     3399       +4     
  Branches      619      621       +2     
==========================================
+ Hits         3040     3044       +4     
  Misses        207      207              
  Partials      148      148              

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

Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

LGTM! @tsalo @handwerkerd what do you think?

@handwerkerd handwerkerd added the breaking change WIll make a non-trivial change to outputs label Nov 29, 2023
@handwerkerd
Copy link
Member

Thank you for taking care of this.
I just opened a PR to @martaarbizu's branch to update the API documentation to reflect these changes.

I'd want @tsalo's option, but I'd lean against changing optcomDenoised to denoised. Other programs that use these file names, like fMRIPrep & personal scripts, would likely need to make a change to properly process files. I like denoised slightly more, but both names are fine and I'll lean against a change that will break other people's scripts unless we have a strong reason to do so.

That said, I am labeling this as a "breaking change" since we are altering default outputs (even if they are default outputs no one actually uses)

@tsalo
Copy link
Member

tsalo commented Nov 29, 2023

I'd want @tsalo's option, but I'd lean against changing optcomDenoised to denoised. Other programs that use these file names, like fMRIPrep & personal scripts, would likely need to make a change to properly process files. I like denoised slightly more, but both names are fine and I'll lean against a change that will break other people's scripts unless we have a strong reason to do so.

I believe that I had this filename change in my version as well. FWIW, fMRIPrep only uses the t2smap workflow, so it doesn't produce/use the denoised file. No matter what, this will be a breaking change (there's always the possibility that some users are using the files that this PR removes), so I say we go for broke and rename everything we think would benefit from a change.

@handwerkerd
Copy link
Member

@tsalo Does BIDS care if denoised is or is not capitalized? If not, once my additions to the comments are added, I'm fine with this merging. When we release the next version, we should make a non-trivial note that we've changed a file name because that will affect people.

@tsalo
Copy link
Member

tsalo commented Nov 29, 2023

There's no rule about the naming, as long as we stick to alphanumeric strings. I personally prefer lower-case or lowerCamelCase, but that's not required at all.

@handwerkerd
Copy link
Member

@martaarbizu Thank you again for working on this. I made a pull request onto your PR to improve the documentation. You should be able to approve and merge martaarbizu#1 into your PR and then this will be ready to merge into Main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change WIll make a non-trivial change to outputs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove accepted and rejected time series from the standard file outputs
4 participants