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.

Sign up for GitHub

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

Jump to bottom

Create bids_ignore directory and redirect channels plot, trigger plot, logger output and subject log file to this directory #245

Merged
merged 25 commits into from
Jun 18, 2020
Merged

Create bids_ignore directory and redirect channels plot, trigger plot, logger output and subject log file to this directory #245

merged 25 commits into from
Jun 18, 2020

Conversation

vinferrer
Copy link
Collaborator

Proposed Changes

  • Create extra folder under the output dir
  • Eliminate channels plot path output option and output channel plot always here, channel plot not optional anymore
  • Eliminate trigger plot path output option and output channel plot always here, channel plot not optional anymore
  • Change logger file output folder to extra
  • Same with subject log file

@vinferrer vinferrer self-assigned this Jun 16, 2020
@vinferrer vinferrer added the Refactoring Improve nonfunctional attributes label Jun 16, 2020
@vinferrer vinferrer requested review from smoia and removed request for smoia June 16, 2020 15:22
@codecov
Copy link

codecov bot commented Jun 16, 2020 •
edited
Loading

Codecov Report

Merging #245 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
- Coverage   93.56%   93.55%   -0.01%     
==========================================
  Files           8        8              
  Lines         730      729       -1     
==========================================
- Hits          683      682       -1     
  Misses         47       47
Impacted Files Coverage Δ
phys2bids/cli/run.py 96.42% <ø> (-0.13%) ⬇️
phys2bids/phys2bids.py 89.74% <100.00%> (+0.08%) ⬆️
phys2bids/viz.py 97.22% <100.00%> (-0.04%) ⬇️

@smoia smoia added Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) and removed Refactoring Improve nonfunctional attributes labels Jun 16, 2020
@vinferrer
Copy link
Collaborator Author

vinferrer commented Jun 16, 2020 •
edited
Loading

Eneko and I were thinking that we should always direct the channels plot, trigger plot, logger output and subject log file. Therefore I am gonna eliminate user output options for channels and trigger plot. Any concerns? @smoia

@smoia
Copy link
Member

smoia commented Jun 16, 2020 •
edited
Loading

By "direct" you mean "create"?
Yes, it can be done. Once per input file rather than one per output.

The only concern is that if you keep your settings constant throughout your acquisition, you don't need to know which channel is which for every file, just one. The channel plot that we output is not for quality check: in longer runs (like multi-scans) we should only have the first N minutes (don't remember how many). So the channel plot may be redundant, but if it's lightweight it's fine.

@eurunuela
Copy link
Collaborator

@vinferrer please make it ready for review if it actually is ready for review 😄

@vinferrer
Copy link
Collaborator Author

i was wating for travis

@vinferrer vinferrer marked this pull request as ready for review June 17, 2020 09:01
@vinferrer
Copy link
Collaborator Author

There seems to be a problem in the azure test:
Could not get the latest source version for repository physiopy/phys2bids hosted on https://github.com/ using ref 0d4ed8d325a575a3534a09c8510af13d242b3a00. GitHub reported the error, "An error occurred while sending the request."
Can we rerun it?

@eurunuela
Copy link
Collaborator

I think @tsalo mentioned the Azure issue yesterday. Let me see if I have the same issue with my PR and I'll get back to you.

@eurunuela
Copy link
Collaborator

@vinferrer why don't you make an empty commit to run the tests again? It's very weird that your test ran for less than 1 second in Azure. It's running on #244 and the error @tsalo mentioned was CodeCov related I think.

@vinferrer vinferrer changed the title create extra and redirect channels plot, trigger plot, logger output and subject log file create extra folder and redirect channels plot, trigger plot, logger output and subject log file Jun 17, 2020
eurunuela
eurunuela reviewed Jun 17, 2020
View reviewed changes
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.

I'm happy with the PR. I just don't know if the name of the folder is the right one... Hoping we can have a short discussion to decide what's the best name for it.

phys2bids/phys2bids.py Outdated Show resolved Hide resolved
@smoia smoia assigned eurunuela and unassigned vinferrer Jun 17, 2020
@vinferrer vinferrer self-assigned this Jun 17, 2020
smoia
smoia requested changes Jun 17, 2020
View reviewed changes
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Looks good! A couple of changes more and it's good to be merged!

phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
eurunuela
eurunuela requested changes Jun 17, 2020
View reviewed changes
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.

Minor comments

phys2bids/phys2bids.py Outdated Show resolved Hide resolved
phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
phys2bids/tests/test_integration.py Outdated Show resolved Hide resolved
@smoia
Copy link
Member

smoia commented Jun 17, 2020

@vinferrer I just solved a conflict - pull from your remote before starting again!

@vinferrer
Copy link
Collaborator Author

Okay I Think this is good to go

@eurunuela eurunuela changed the title create extra folder and redirect channels plot, trigger plot, logger output and subject log file Create bids_ignore directory and redirect channels plot, trigger plot, logger output and subject log file to this directory Jun 18, 2020
eurunuela
eurunuela approved these changes Jun 18, 2020
View reviewed changes
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! Thanks @vinferrer !

@smoia smoia self-requested a review June 18, 2020 08:37
smoia
smoia approved these changes Jun 18, 2020
View reviewed changes
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

LGMT!!!

@vinferrer vinferrer merged commit e5ae06a into physiopy:master Jun 18, 2020
@vinferrer vinferrer deleted the extra_folder branch June 18, 2020 09:55
@smoia smoia added the released This issue/pull request has been released. label Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoia smoia smoia approved these changes

@eurunuela eurunuela eurunuela approved these changes

Assignees

@eurunuela eurunuela

Labels
Minormod This PR generally closes an `Enhancement` issue. It increments the minor version (0.+1.0) released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vinferrer @smoia @eurunuela