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 #6

Closed
cumbof opened this issue Jul 3, 2023 · 5 comments
Closed

JOSS Review #6

cumbof opened this issue Jul 3, 2023 · 5 comments

Comments

@cumbof
Copy link

cumbof commented Jul 3, 2023

Hello,

I have a few comments concerning your python package and your manuscript:

  1. let's start with the installation process. I honestly don't see a reason why this package shouldn't be available in the Python Package Index https://pypi.org. I would strongly suggest to create a pypi package. You already have everything to do that;
  2. before uploading the package to pypi, I would definitely refactor the setup.py. Please add as many details as possible (see this example here);
  3. this is the first time I see taking track of a tool version through a .VERSION file. Is that used for any other purpose? Is it used by a GitHub action? If not, I would add this info in the pymgpipe/__init__.py, i.e., __version__ = "v0.16.0". At that point, I would add from pymgpipe import __version__ in the setup.py to access the software version, without the need to read a file. And you can get rid of the .VERSION file now;
  4. now that you have your package available in pypi, it would be extremely easy to define a Bioconda recipe. At that point, once you have a conda package, you don't have to take care of installing the external dependencies by your own anymore;
  5. on line 65-66, you said that The pymgpipe python package, as well as all associated documentation, tests, and example workflows, can be found at https://korem-lab.github.io/pymgpipe. This is not correct since that page shows the code documentation only. I would reformulate that sentence;
  6. the code documentation seems not complete. Most of the functions are not documented. This must be fixed;
  7. that's great that you provide a Notebook to better explain users how to use the library. The problem here is the lack of notes. I can only see python codes here, but there is no explanation about what you are doing. Please, add a text explaining what you do in every cell of the notebook.

Please, do not close this thread.
I'll update it in case I will have any other comments.

@ym2877
Copy link
Collaborator

ym2877 commented Jul 3, 2023

@cumbof thank you for taking the time to review our package. These points are very helpful- I will try my best to address each of them over the next couple of days.

@ym2877
Copy link
Collaborator

ym2877 commented Jul 6, 2023

@cumbof I have made the suggested changes and updated the joss branch as well. Please don't hesitate to let me know if you have any additional questions, comments, concerns. Thank you!

@cumbof
Copy link
Author

cumbof commented Jul 7, 2023

Seems great overall. Thanks for working on my comments @ym2877.

I only have a couple of extra comments:

  1. you addressed point 3 by moving the .VERSION file into the pymgpipe/resources as specified in the setup.py file. However, I don't actually see any .VERSION file under that folder. I'm ok with taking track of the package version with the .VERSION file instead of specifying it in the pymgpipe/__init__.py file as I suggested. Just remember to push that file into the resources folder;
  2. you should add a few sentences in the README.md about how you handle people's contributions (since this is open source) and how people should report issues. Look at the example here;
  3. what about point 4? That's a very minor point compared to the others and it would be enough to have the package in PyPI. However, making it available in bioconda would be way better and it will allow people to avoid taking care of installing the external software dependencies by their own.

Point 1 and 2 must be addressed, while point 3 is strongly recommended but not mandatory.

@ym2877
Copy link
Collaborator

ym2877 commented Jul 7, 2023

Okay, addressed points #1 and #2! Thanks again for your comments @cumbof. Point #1 was just an error, that reference to .VERSION was old and shouldn't have been there, so thank you for catching that!

With regard to #3, I will absolutely look into this. Although, with the way our dependencies are currently set up in setup.py, there is no need for users to install external dependencies on their own. The only steps they would need to run is

  1. conda create -n my_env python=3.10
  2. conda activate my_env
  3. pip install pymgpipe

Maybe I am misunderstanding what you mean. Either way, I will look into publishing a bioconda package asap!

@cumbof
Copy link
Author

cumbof commented Jul 7, 2023

Thanks for your quick reply @ym2877.

That's true, your tool doesn't have any non-python dependencies actually and everything is handled by pip. However, I would consider adding your tool to bioconda in case anyone wants to use your tool as a dependency along with other non-python tools.

Again, I don't consider this point as mandatory in the context of the JOSS review, but I would strongly recommend to consider addressing this extra step. Defining a conda recipe would be extremely easy at this point.

I'm going to close this issue since I don't have any other comments.

@cumbof cumbof closed this as completed Jul 7, 2023
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

2 participants