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

ENH: Interface for R #3291

Merged
merged 19 commits into from
Jun 2, 2021
Merged

ENH: Interface for R #3291

merged 19 commits into from
Jun 2, 2021

Conversation

Terf
Copy link
Contributor

@Terf Terf commented Jan 11, 2021

Summary

Like matlab, R has many useful neuroimaging tools but isn't as nice of a scripting language as python. This PR creates an R interface akin to the matlab interface, so R code may be more easily wrapped by nipype.

List of changes proposed in this PR (pull-request)

  • Added R interface

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

mostly a copy-paste of the matlab interface
@codecov
Copy link

codecov bot commented Jan 11, 2021

Codecov Report

Merging #3291 (e576c90) into master (337bbc9) will increase coverage by 0.02%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3291      +/-   ##
==========================================
+ Coverage   65.05%   65.07%   +0.02%     
==========================================
  Files         306      307       +1     
  Lines       40279    40327      +48     
  Branches     5320     5326       +6     
==========================================
+ Hits        26202    26242      +40     
- Misses      13006    13012       +6     
- Partials     1071     1073       +2     
Flag Coverage Δ
unittests 64.80% <45.83%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
nipype/interfaces/r.py 83.33% <83.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 337bbc9...e576c90. Read the comment docs.

@effigies effigies mentioned this pull request May 26, 2021
14 tasks
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Overall looks good. I started reviewing this a while back, but I guess got lost in my sea of tabs. Here are some suggestions, and could you make a test (in nipype/interfaces/tests/test_r.py) that skips if R is not found? e.g.:

@pytest.mark.skipif(no_r, "No R command found")
def test_basic_Rscript():
    ...

@effigies
Copy link
Member

@Terf I'm going to try to make a release in the next couple days. If you want to try to finish this off, I'd love to include it!

Terf and others added 3 commits May 27, 2021 23:18
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@Terf
Copy link
Contributor Author

Terf commented May 28, 2021

@effigies awesome! I just committed some tests, let me know if there any problems

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

You're getting some test failures that should be fixed in master, if you want to merge.

Terf and others added 9 commits May 31, 2021 19:19
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@effigies
Copy link
Member

effigies commented Jun 1, 2021

Can you merge latest master and run make specs? That should fix the failing tests.

@effigies effigies added this to the 1.6.1 milestone Jun 1, 2021
Copy link
Member

@effigies effigies 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 good with this, at this point. I notice that the outputs are pretty verbose, so I suspect that at some point we'll want to add inputs to allow for control of that. If you want to adjust the behavior at all before merge, I'm happy to wait. Otherwise we can merge now and improve later.

@effigies effigies merged commit 5d2e224 into nipy:master Jun 2, 2021
@effigies
Copy link
Member

effigies commented Jun 2, 2021

Awesome, thanks. If you're not already in the Zenodo file and would like to be included as an author on releases, please post on #3336 with your name, affiliation and ORCID.

@Terf
Copy link
Contributor Author

Terf commented Jun 3, 2021

Since this PR’s already merged I’ll submit a new PR with inputs to control the verbosity of outputs. Right now, calling run() on an RCommand will return a “Bunch” object with fields like the command line executed, command output, env var dump, timestamps, etc. Perhaps the less verbose version should just return the command output as the command would?

I wonder if these changes should just apply to the R interface, or we should abstract the R and matlab interfaces into an “AbstractLanguage” class that could be extended by other language interfaces to reduce code reuse. After all, the R interface copied much of the code from the matlab interface, and I think it could be useful to have interfaces for Java and Julia which would worsen the code duplication problem between them.

That'd be great if I could be included as an author on releases! Thanks, and I hope to make more significant contributions in the future.

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