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

[X19V] refactorisation #93

Closed
wants to merge 20 commits into from
Closed

Conversation

Remi-Gau
Copy link
Contributor

@Remi-Gau Remi-Gau commented Sep 20, 2023

This Pull Request is related to issue None

Changes proposed in this Pull Request:

  • use the Pipeline class for the analysis of the team x19v
  • add tests for it
  • adapt notebook
  • add a couple of fixtures inc conftest

Checklist:

  • Descriptive title
  • Targets the main branch
  • Changes are functional
  • My code is explicit and comments were added to it
  • Code conforms with PEP8
  • Tests were added for the changes and they complete successfully
  • Existing tests were updated (if needed) and they complete successfully
  • Documentation was updated

@Remi-Gau Remi-Gau changed the title [MAINT] refactor x19v [X19V] refactorisation Sep 21, 2023
@Remi-Gau Remi-Gau marked this pull request as ready for review September 21, 2023 15:42
Copy link
Contributor Author

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

Need to take into account excluded particiants from narps_open/data/description/analysis_pipelines_derived_descriptions.tsv

Actually filtering out excluded participant should probably be part of the subject_list setter in the Pipeline class.

@@ -413,10 +413,10 @@ def get_subgroups_contrasts(
if sub_id[-2][-3:] in subject_list:
varcopes_global.append(varcope)

return copes_equal_indifference, copes_equal_range,
return (copes_equal_indifference, copes_equal_range,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove changes as they are in #92

Comment on lines +1 to +2
[tool.isort]
combine_as_imports = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably better suited in a separate pr

@@ -7,26 +7,27 @@
"""

from os import remove
from os.path import join, isfile
from os.path import isfile, join
from pathlib import Path
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe only keep the changes thst relevant to this PR and not all the black related things.


@fixture
def participant_tsv(bids_dir) -> Path:
"""Return the participant tsv file as a DataFrame."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I used the fixture, but there may be a use case for a method in the Pipeline class to facilitate access to the actual file.


subgroups_contrasts = Node(
Function(
input_names=["copes", "varcopes", "subject_ids", "participants_file"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"subject_ids", "participants_file"

those are not needed anymore: make sure that all Node definition are updated

@bclenet
Copy link
Collaborator

bclenet commented Feb 19, 2024

Closing this PR, after merging PRs #153 and #177.

@bclenet bclenet closed this Feb 19, 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.

2 participants