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

convert convertxfm.py #33

Merged
merged 79 commits into from
Sep 12, 2022
Merged

convert convertxfm.py #33

merged 79 commits into from
Sep 12, 2022

Conversation

yibeichan
Copy link
Collaborator

hello, I manually converted convertxfm.py, hope it's correct and can pass the test

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022

Codecov Report

Merging #33 (cca0b1f) into master (094cec2) will decrease coverage by 0.41%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master     #33      +/-   ##
=========================================
- Coverage    0.87%   0.45%   -0.42%     
=========================================
  Files          25      33       +8     
  Lines         343     656     +313     
  Branches       43     126      +83     
=========================================
  Hits            3       3              
- Misses        340     653     +313     
Impacted Files Coverage Δ
pydra/tasks/fsl/model/cluster.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/model/feat.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/model/featmodel.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/model/filmgls.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/model/flameo.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/model/glm.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/model/melodic.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/utils/complex.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/utils/convertxfm.py 0.00% <0.00%> (ø)
pydra/tasks/fsl/utils/filterregressor.py 0.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@yibeichan
Copy link
Collaborator Author

okay... I'll stop creating more failed test reports. @djarecka I'll see you in your office hour tomorrow morning around 7am (your 10 am).

@djarecka
Copy link
Contributor

djarecka commented Jul 6, 2022

@yibeichan - the tests were failing even before, it's not your fault. We should fix it.

Also, tomorrow I can also be around 11:30 ET if that works better for you

@yibeichan yibeichan changed the title manually convert convertxfm.py convert convertxfm.py Jul 12, 2022
@yibeichan
Copy link
Collaborator Author

@djarecka finally!!! I passed all tests, except the docker one. I had some issues with doctest.
FSL generates tmp outputs and have them in the cmdline. This causes the difference between my input cmdline and doctest generated cmdline. Only two model/cluster.py and model/glm.py have this issue. So I add #doctest: +SKIP for those two files.
After merge this pr, I'll work on

  1. make converter.py more specific
  2. write pydra tasks for interfaces which don't have _cmdline

@yibeichan
Copy link
Collaborator Author

@djarecka as we just discussed, I changed settings in related to bet.py, cluster.py, and flameo.py and they passed the test locally but still failed the docker here.
Also, this time the docker test has 17 failures, I'm pretty sure I didn't change other files.
the errors are the same Error: short read, file may be truncated

@yibeichan
Copy link
Collaborator Author

@djarecka I changed ty.Any to ty.Union[bool, str] and set the default value as False in cluster.py https://github.com/yibeichan/pydra-fsl/blob/cca0b1f96a38fed6cbaad2b48a3391f63413e486/pydra/tasks/fsl/model/cluster.py#L32
The test has failed. Similar errors as other failed tests which don't need ty.Union[bool, str]

E               stderr:
E               terminate called after throwing an instance of 'NiftiIO::NiftiException'
E                 what():  Error: short read, file may be truncated

We can talk about this tomorrow 10am (EST)/7am (PST), via jit.si

@djarecka
Copy link
Contributor

djarecka commented Aug 4, 2022

that is interesting, we can look at this tomorrow.

I've change the FNIRT interface/test, but still not passing, trying to figure out what I misunderstood how the interface works

@yibeichan
Copy link
Collaborator Author

yibeichan commented Aug 9, 2022

@djarecka hello, just one more thing I noticed. Remember that I have FSL installed on my Mac so I don't need to use docker to do testing, and I passed all tests locally but failed checks here. So, I pulled your docker image, followed the workflow here, and ran all tests in the docker. The same working branch, same pydra version, I passed all tests locally...
Maybe there are also some issues with the docker test online? We can talk tomorrow morning!

@djarecka
Copy link
Contributor

@yibeichan - let fix the issues with test in docker in a separate PR, will merge it now! Great work, thank you!

@djarecka djarecka merged commit 6957eb6 into nipype:master Sep 12, 2022
@djarecka djarecka mentioned this pull request Sep 12, 2022
@djarecka
Copy link
Contributor

wait, I forgot that we figured this out and I can see that you opened another PR...

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.

None yet

3 participants