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

JOSS review - current state & integration to NiBabel #118

Closed
PeerHerholz opened this issue Jul 12, 2021 · 9 comments
Closed

JOSS review - current state & integration to NiBabel #118

PeerHerholz opened this issue Jul 12, 2021 · 9 comments

Comments

@PeerHerholz
Copy link

Ahoi hoi folks,

this is in reference to the review I'm currently conducting for JOSS (here).

First of all, thank you very much for your dedicated work on this tool that definitely is much needed and fills an important gap in the python neuroimaging realm. One central question I was wondering about and targets a lot of checkboxes in the review process is the current state & planned integration into NiBabel (#110 here & #656 in NiBabel). I understand that the general plan is to integrate NiTransforms as a module into NiBabel. However, NiTransforms' current state appears a bit nebulous concerning its place and documentation. Regarding the first, it might be helpful to clearly state that NiTransforms evolved out of a planned addition to Nibabel to its own repository and package for development reasons, but upon finalization will be integrated back into NiBabel. This blends into the second point, documentation. While I assume that functionality and API will be part of the NiBabel docs after the planned merge (with installation instructions then becoming obsolete), it would still be necessary to have these aspects/resources during the transition period, given the review process and to allow interested parties to use the software. I saw a docs folder and docs related PRs, but wasn't able to find a respective link. Would it be possible to provide a link for the documentation and maybe update the README to include the corresponding points?

I'm very sorry if I missed something obvious, misunderstood something or assumed something wrong. Please let me know if that is the case, as well as if you have any questions. Thanks again for creating this important software tool.

Cheers, Peer

@mgxd
Copy link
Member

mgxd commented Jul 13, 2021

Hi Peer!

You are right, docs are not being advertised! I added the link as the repo's website (https://nitransforms.readthedocs.io), but when browsing through noticed the API section is not populating. I'll take a look into that now.

@oesteban can you add me as a maintainer on RTD?

@PeerHerholz
Copy link
Author

Hi @mgxd,

thank you very much for your quick reply and the information!
Ok, coolio, sounds great. Would you mind letting me know when you finished that work and I should have a look at it?

Thanks again.

Cheers, Peer

@mgxd
Copy link
Member

mgxd commented Jul 13, 2021

Should be all set after #120 - https://nitransforms.readthedocs.io/en/latest/api.html

@oesteban
Copy link
Collaborator

oesteban commented Jul 13, 2021 via email

@oesteban
Copy link
Collaborator

I understand that the general plan is to integrate NiTransforms as a module into NiBabel. However, NiTransforms' current state appears a bit nebulous concerning its place and documentation. Regarding the first, it might be helpful to clearly state that NiTransforms evolved out of a planned addition to Nibabel to its own repository and package for development reasons, but upon finalization will be integrated back into NiBabel.

Yes, this is a great point. NiTransforms was envisioned as part of NiBabel since the beginning. However, shortly after starting out as a PR to NiBabel, we realized that it was going to build up in a humongous PR nobody would be able to review as thoroughly as it would require. Also, NiTransforms has many connections to BIDS/BIDS-Derivatives and its X5 format specification for transforms, which falls outside of the current scope of NiBabel.

So the plan is to make it an isolated tool, and once it is finished, integrate with NiBabel. On its end, NiBabel just initiated its CZI EOSS, so there's quite a lot of work ahead refactoring the library. The grant also covers aspects of NiTransforms. Once NiTransforms is ready for integration, we will define what can go in NiBabel (presumably everything, except perhaps some final details of the X5 implementation, although NiBabel will support the data structure at least logically).

This is to say that the chances that NiTransforms is integrated into NiBabel are high and scheduled to happen in ~10 months. The "bus factor" is high in this case thanks to the CZI funding. However, we believe NiTransforms can be considered as a software component of its own that deserves a focused publication at JOSS.

Regarding documentation, there's a whole lot of action items in the CZI grant for NiBabel (and NiTransforms too). When the implementation of all planned features is concluded, we will start generating tutorials. Some of them may also be covered by other funds we are trying to put together around NiPreps (www.nipreps.org).

Finally, we are planning to transfer over the repo under nipy (or at least, nipreps). We haven't done so this far as the poldracklab organization uses fewer resources from GitHub Actions and CircleCI at the moment. However, if you believe this would be important for the visibility of the project or other reasons I cannot think of right now, we are happy to push the transfer sooner than expected.

@oesteban
Copy link
Collaborator

However, we believe NiTransforms can be considered as a software component of its own that deserves a focused publication at JOSS.

Which doesn't mean we will not follow a long deprecation cycle for those who try the code as in the JOSS publication. Importing from nitransforms will be supported for a long time (as a shim that uses nibabel underneath) until we are sure everyone is using nibabel's import directly.

@PeerHerholz
Copy link
Author

Hi @oesteban,

thank you very much for your response and the additional information.
IMHO the plan/ideas outlined by you are very reasonable and understandable. I completely agree with NiTransforms' role as a dedicated software package/tool that definitely deserves its own publication. My comments were rather targeted at including this information in the docs and potentially manuscript. I'm very sorry for this misunderstanding.

P.S.: The entire work package around NiBabel's CZI seems fantastic!

@PeerHerholz
Copy link
Author

Hi @mgxd (& @oesteban),

thanks a lot for updating the docs and adding the link to the repo.
Would it be possible to discuss a few further things? All the points I would like to bring up are actually perfectly summarized by @robbisg in the respective issue: installation, examples and contribution guidelines. Thus, I'll just point to the comments outlined there. One small general addition that would be cool: a CoC and one potential addition to the docs: the spatial transforms section of the paper, as it would add some great background information there as well.

@mgxd
Copy link
Member

mgxd commented Sep 22, 2021

thanks again @PeerHerholz !

@mgxd mgxd closed this as completed Sep 22, 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

No branches or pull requests

3 participants