Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Remove non-alphanumeric characters from workflow names and output entities #434

Merged
merged 3 commits into from
May 30, 2024
Merged

Remove non-alphanumeric characters from workflow names and output entities #434

merged 3 commits into from
May 30, 2024

Conversation

tsalo
Copy link
Contributor

@tsalo tsalo commented Apr 2, 2024 •
edited
Loading

Closes #295.

Changes proposed:

  • Use regular expressions to remove non-alphanumeric characters from the B0FieldIdentifier used in workflow names and output entities.

@codecov Codecov
Copy link

codecov bot commented Apr 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 76.41%. Comparing base (2456489) to head (a18cdb2).

Files Patch % Lines
sdcflows/workflows/fit/base.py 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   76.43%   76.41%   -0.03%     
==========================================
  Files          32       32              
  Lines        2835     2841       +6     
  Branches      376      376              
==========================================
+ Hits         2167     2171       +4     
- Misses        600      602       +2     
  Partials       68       68

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

@marcelzwiers marcelzwiers mentioned this pull request Apr 16, 2024
Closed
@effigies effigies merged commit b10de08 into nipreps:master May 30, 2024
22 checks passed
@tsalo tsalo deleted the fmapid branch May 30, 2024 19:25
@mgxd
Copy link
Contributor

mgxd commented May 30, 2024

This is causing an error upstream:

Traceback (most recent call last):
  File "/opt/conda/envs/nibabies/bin/nibabies", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/conda/envs/nibabies/lib/python3.11/site-packages/nibabies/cli/run.py", line 61, in main
    retval = build_workflow(config_file)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/nibabies/lib/python3.11/site-packages/nibabies/cli/workflow.py", line 76, in build_workflow
    retval['workflow'] = init_nibabies_wf(config.execution.unique_labels)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/nibabies/lib/python3.11/site-packages/nibabies/workflows/base.py", line 146, in init_nibabies_wf
    single_subject_wf = init_single_subject_wf(
                        ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/nibabies/lib/python3.11/site-packages/nibabies/workflows/base.py", line 589, in init_single_subject_wf
    wf_inputs = getattr(fmap_wf.inputs, f'in_{estimator.bids_id}')
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'TraitedSpec' object has no attribute 'in_auto_00000'

We probably want to either change estimator.bids_id to ensure it is cleaned, or add a new attribute to easily access the cleaned id.

@effigies
Copy link
Member

Let's revert. We can rethink this.

@effigies effigies mentioned this pull request May 30, 2024
Merged
@effigies
Copy link
Member

Reverted. What if we replace all non-alphanumeric characters with underscores?

@mgxd
Copy link
Contributor

mgxd commented May 31, 2024

Removing or replacing both sound reasonable - it's just the sanitized id needs to be available within the FieldmapEstimation class so we can easily index the workflow inputs.

This was referenced Jun 5, 2024
Open
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@effigies effigies effigies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

B0FieldIdentifier: some characters should be replaced to set workflow name to avoid crashes in graph creation
3 participants
@tsalo @mgxd @effigies