Joss review comments, concerns and suggestions.

[curtcorum]

@jmontalt, @jennifersteeden, @pascalemp

@cncastillo, @paquiteau, @Kevin-Mattheus-Moerman

Checklist: openjournals/joss-reviews#8410 (comment)

First I concur with ALL the comments of reviewer @paquiteau: openjournals/joss-reviews#8410 (comment)

Especially:

I would recommend to move to pyproject.toml setup; Using a setup.py is deprecated

Also of note:

README.rst is not up to date, latest released version is v0.23.1

Ideally the JOSS paper references a specific release and potentially changes made in review, should this be v0.23.1? Upcoming v0.23.2?

It looks like there are a lot of changes between the current main head 98c0fde and v0.23.1, was this a forced update or other variation? There is too much to track: v0.23.1...master

[curtcorum]

As an experiment I used copilot to generate a checklist:

JOSS Submission Checklist for curtcorum/tensorflow-mri

Use this checklist to ensure your repository meets the JOSS guidelines before submission.

Some of it was probably incorrect based on expecting a README.md. I pruned it to items I agree with:

Documentation

• README.md at repository root includes:

  • Project overview and scientific purpose (some description, but unfocused)
  • Statement of need (scientific problem addressed, target audience) (some description, but unfocused)
  • Installation instructions out of date setup.py
  • Usage examples exist but not linked directly in README.md
  • Links to documentation and related work no Docs for v.23.1
  • Citation instructions (BibTeX/DOI) Zenodo links not up to date for up to v0.23.1
  • License and authorship information

• API Documentation (if applicable) need to update

  • Generated docs (e.g., Sphinx for Python, JSDoc for JS/TS)
  • Usage and function/class descriptions

• Changelog (CHANGELOG.md) at root documents user-facing changes and recent releases

Project Management

• Contribution guidelines (CONTRIBUTING.md) at root

  • Coding style and conventions
  • Instructions for submitting issues and pull requests
  • How to run tests
  • Reference to the Code of Conduct

• Code of Conduct (CODE_OF_CONDUCT.md) at root

• Authors and affiliations listed in README or AUTHORS.md

Software Quality

• Consistent coding style (e.g., PEP8 for Python, ESLint for JS/TS)
• Linters and code formatting tools are used and documented
• Automated tests included and described
• Instructions for running tests provided
• Continuous integration configured (e.g., GitHub Actions)

JOSS-specific

• License file (LICENSE) at root and mentioned in README
• Citation file (CITATION.cff or instructions in README)
• Statement of need clearly present in documentation
• Related work referenced and compared

Other

• Project directories and files clearly organized
• Remove unnecessary template/boilerplate files
• Semantic versioning used and documented in changelog

---

Adapted for JOSS review based on https://joss.theoj.org/papers/instructions

[curtcorum]

Malformed link in README.rst, possibly works in README.md?

[![Watch the video](tools/assets/thumb.png)](https://vimeo.com/1054518675/e19c8abad3)

[curtcorum]

With current pip based setup it is probably most appropriate to give or at least point to instructions for setting up a venv:

https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/

[curtcorum]

Also supported (and tested) python versions. #50

[jmontalt]

Dear @curtcorum ,

Thank you so much for your review and your valuable comments. I agree with virtually everything. Here are some of my first thoughts:

• setup.py: while I also prefer pyproject.toml, we will need to find a way to adapt the custom packaging workflow used to compile native C++ ops.
• Regarding Python versions: we need to upgrade the C++ ops to support modern Python versions. This is complicated by recent changes in how TensorFlow binds to custom ops, but in my opinion should take maximum priority.
• I believe some of the automated workflows (eg, the deployment of the latest docs) were not correctly triggered. The release process needs to be better documented or made more robust to human error.
• Some mistakes seem to have happened in recent releases and in the master branch, which caused the failed builds and explain the deviation between master and the latest release.

I will be in touch with my co-authors to find a way to address these and all other points. However, I expect this will take some time, not least because I work elsewhere now and my availability is limited. Is there a deadline?

[jmontalt]

Malformed link in README.rst, possibly works in README.md?

[![Watch the video](tools/assets/thumb.png)](https://vimeo.com/1054518675/e19c8abad3)

Indeed this is Markdown syntax, not RestructuredText, which would explain the error. Thank you for pointing this out!

I would personally prefer to use .md files everywhere (more common, easier to write), but this has implications on the docs build workflow. Back then, I had to work around various limitations of Sphinx and associated extensions, but I suspect there may be better ways to do this nowadays. It might be a good idea to re-assess this soon.

[curtcorum]

@jmontalt, @jennifersteeden, @pascalemp

@cncastillo, @paquiteau, @Kevin-Mattheus-Moerman

Review Summary:

Clearly tensorflow-mri has been and is a powerful package which would be a great addition to the general mri community. Unfortunately the package currently needs much updating, for consistency (between documentation and repository) and more testing for robustness in various linux and python environments.

Currently it does not seem generally installable or compatible without excessive effort, luck, or help from the developers.

The documentation in the repository is incomplete and it seems that previously functioning notebooks have been removed from the current repository. The previous documentation and examples are quite extensive. It is not clear where the source for this resides, and ideally is a part of the up to date repository.

I'd like to thank the developers for their significant past and current efforts, but in this reviewer's opinion, much work is needed to bring the project up to joss standards.

[curtcorum]

@jmontalt I do not believe there is a specific deadline, editor @Kevin-Mattheus-Moerman should be able to comment.

Again, I think bringing tensorflow-mri up to joss standards would be a significant contribution to the mri open source community!

[curtcorum]

@jmontalt, @jennifersteeden, @pascalemp

@cncastillo, @paquiteau, @Kevin-Mattheus-Moerman

Dear all, I believe the authors are still working/responding on the issues brought up by the reviewers.

#51 (comment)

Dear @curtcorum ,

Thank you so much for your review and your valuable comments. I agree with virtually everything. Here are some of my first thoughts:

* `setup.py`: while I also prefer `pyproject.toml`, we will need to find a way to adapt the custom packaging workflow used to compile native C++ ops.

* Regarding Python versions: we need to upgrade the C++ ops to support modern Python versions. This is complicated by recent changes in how TensorFlow binds to custom ops, but in my opinion should take maximum priority.

* I believe some of the automated workflows (eg, the deployment of the latest docs) were not correctly triggered. The release process needs to be better documented or made more robust to human error.

* Some mistakes seem to have happened in recent releases and in the master branch, which caused the failed builds and explain the deviation between master and the latest release.

I will be in touch with my co-authors to find a way to address these and all other points. However, I expect this will take some time, not least because I work elsewhere now and my availability is limited. Is there a deadline?

#51 (comment)

Malformed link in README.rst, possibly works in README.md?
[![Watch the video](tools/assets/thumb.png)](https://vimeo.com/1054518675/e19c8abad3)

Indeed this is Markdown syntax, not RestructuredText, which would explain the error. Thank you for pointing this out!

I would personally prefer to use .md files everywhere (more common, easier to write), but this has implications on the docs build workflow. Back then, I had to work around various limitations of Sphinx and associated extensions, but I suspect there may be better ways to do this nowadays. It might be a good idea to re-assess this soon.

openjournals/joss-reviews#8410 (comment)