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

NF improve converter and utils interface that can be handled #11

Merged
merged 30 commits into from
Jun 10, 2021

Conversation

htwangtw
Copy link
Collaborator

@htwangtw htwangtw commented Apr 15, 2021

Changes to converter.py:

  • add default help string when nipype1 provides none
  • inputs_drop allow converter dropping input specs that is not compatible with pydra
  • ensure name_source grabbed from nipype1 is a list
  • allow using callable when encountering nipype1 genfile in input spec

Remaining issues for future PRs:

  • implement parser for argstr: a series of float ("%f %f") and %g
  • parser for plain text output (possibly a pydra general function; see AvScale)
  • expand test data type (ConvertXFM)
  • Some dropped input spec should be implemented as pydra task
    • fwhm to sigma conversion (Smooth)
    • number pair to proper input (ExtractROI)

To-do:

  • fix docstring path issue
  • find test data?

htwangtw and others added 24 commits February 23, 2021 15:21
* 'master' of https://github.com/nipype/pydra-fsl:
  MNT: Find namespace packages
  FIX: Find packages correctly
  Update README.md
  using lru_cache for all_interface
  adding specs dir to the python path in the converter
  Update tools/converter.py
  updating readme
  using all as a default option for interface_name in cli; checking name of modules and available interfaces
  updating readme
  adding cli to the converter
Still need to work on the test

* fix-converter:
  fix yml module name input
Still need to understand how test conversion works.
Need to add customised components to converter
FIX: Add help_string by default
Pydra callable only applies for output manipulation, but fslmaths
requires a output file name. Nipype autogenerate a out_file name when
there's no input. Not sure how to resolve this in pydra.
Allow converter to drop a given list of nipype interface parameter.
Need to add test to check the dropped parameter that's not a
requirement of the fsl command.
… fslmaths

* 'fslmaths' of https://github.com/htwangtw/pydra-fsl:
  ADD converter yml field inputs_drop
  BK fslmaths requires out_file field
  FIX: Add help_string by default
  TEST/BK test generation unsuccessful
  ADD template for all FSL utils
  ADD test in ImageMaths spec
  fix yml module name input
  FIX converter module name
  BK utils yml
  test converter
  ADD first try on fslmaths interface
Possible implementation: pydra callable
Need to check how to do it the pydra way and possibly
implement in the converter
Logging interfaces with tests passed for now:
* copygeom
* extractroi
* filterregressor
* imagemeants
* imagestats
* smooth
attribute name_source in nipype can be string or list.
Make sure when pass to the converter, it's a list
Remaining issues:
* implement parser for argstr: a series of float ("%f %f") and %g
* parser for plain text output (possibly a pydra general function; see AvScale)
* expand test data type (ConvertXFM)
Quotation mark in the converter doctest generator produces error for
filerregressor.py
@djarecka
Copy link
Contributor

@htwangtw - I was checking the test that is failing - you should probably use "sep": " ,", in the input_spec, however it won't work with "argstr": "-f '{filter_columns}'". There is one thing to fix in pydra, and I'm planning to work on this tomorrow, but I'm also not completely sure why are you using '{filter_columns}'. Does the command line requires '' around 1,2,3?

@htwangtw
Copy link
Collaborator Author

Thanks for looking into this!

I was writing the test case according to the doc:

Usage: 
fsl_regfilt -i <input> -d <design> -f <component numbers or filter threshold> -o <out> [options]

...
Optional arguments (You may optionally specify one or more of):
	-m,--mask	mask image file name
	-f,--filter	filter out part of the regression model, e.g. -f "1,2,3" 

It might be fine to remove the "?

@djarecka
Copy link
Contributor

@effigies - I'm confused about nipype interface FAST: in_files use InputMultiPath and have no sep defined. Shouldn't this mean that the input value is always returned as a list?

The test shows that it returns a single value, but I'm not sure why this is a single value if no sep was defined.

@satra
Copy link
Contributor

satra commented May 13, 2021

@djarecka
Copy link
Contributor

oh, ok, so there is default sep value. Do you think we should have in pydra?

@djarecka
Copy link
Contributor

@htwangtw - I've fix the bug in pydra: nipype/pydra#466
but I'm still not sure if the command should have "" around 1,2,3

@htwangtw
Copy link
Collaborator Author

I will update my pydra version to the latest and have a look. I will have a look into the original command

@htwangtw
Copy link
Collaborator Author

The command itself doesn't require " for the input. That was added because that was how this interface was written in nipype. I will add this to my list of "Interface Needing Manual Edit After Conversion"

FIX -
* FAST doc test

BREAK -
* fNIRT - assertion error with output file
* SUSAN - run time error; possibly fixable with more suitable test data
@htwangtw
Copy link
Collaborator Author

htwangtw commented May 27, 2021

Note for @djarecka: Interfaces I worked on all passed the test - this PR is ready to merge if we only want to handle the util module. I can open issues for the remaining issue in the preprocess module. What do you think?

Edit: SUSAN and FNIRT are failing locally now - might be related to my FSL version.

@djarecka djarecka merged commit 51480b3 into nipype:master Jun 10, 2021
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.

4 participants