[JOSS] General Code Comments

[JoaoRodrigues]

Hi molearn team,

Instead of leaving multiple small issues in the repository, I'll organize my comments here. If I have a larger comment or question, I'll open a separate issue and link here.

As a more general comment, I'd recomment using a tool like flake8 or if you're not in the mood to correct things manually, black/ruff, to standardize your code and have it comply a little more with Python style standards. Tiny things like spacing between operands, spacing between functions/docstrings, etc that make the code easier on the eyes for outsiders!

1. Regarding the environment.yml file, I am a little confused if it's meant to help people install from source or not. I see the instructions mention using conda or mamba with -only-deps and then running pip install .. Why not recommending users to simply run conda env create -f environment.yml as a one-step process?

2. Regarding the "no installation" instruction, I'd strongly recommend suggesting pip install -e ., which creates a symlink to the project but does not copy files to the Python installation folder.

3. I don't see a link to the documentation in the website linked to the project. Maybe it would be nice to include it there for simplicity and visibility? Since the docs seem to be mostly API docs, I'd rename them to molearn API docs though.

Assorted comments on code style:

1. You don't need the additional list inside the str.join here. You can also use _ as the variable for the iteration as an indication that you don't actually need it/use it.

2. Your cpu_count function here can be replaced by multiprocessing.cpu_count or os.cpu_count if I read it correctly. That should be more robust.

3. Python 3 classes do not need the class Foo(object) construction, they can be simply class Foo.

4. You should guard these import errors with a variable (for instance _has_modeller) and check it whenever you call a function that calls the dependency. For instance here there is nothing preventing the user from calling this and crashing. So, the import check is a bit useless IMO. You could give the user a nicer message when they try to use the method that needs the dependency. Ideally, you'd move these import checks to utils.py or so. Another suggestion/idea is to move the analysers that depend on each third-party tool to separate files (e.g. analyser_modeler.py) and import those methods in the main analyser.py module. Just to separate the logic a little.

--

Life calls, will continue looking into the code tomorrow!

[degiacom]

@JoaoRodrigues, thank you for your feedback. Here is a summary of our work in progress.

• did a first pass improving code formatting guided by flake8 (also removed some unused imports in the process)
• now all class declaration use Python 3 syntax
• we replaced our custom utils.cpu_count function with os.cpu_count. Thank you for pointing that out, much more robust!
• We had a reference to the documentation website at the bottom of the main Readme file, but it was not immediately findable. We now also refer to it at the top of the page, indicating that the documentation features "API and FAQ".

Better guarding the calls to methods requiring 3rd party software seems useful, we will look into that. Concerning the environment.yml file, it is currently used by .readthedocs.yaml. This said, telling the user to exploit this file to create an environment does not seem like a bad idea to me. @SCMusson, what do you think?

[SCMusson]

Using mamba install -c conda-forge --only-deps molearn installs only the core dependencies and we have access to the conda-forge CI to confirm these are working (especially since we don't currently have our own CI setup yet). This is useful for a lean/minimum install and to avoid licensing issues (e.g. modeller licenses). We can add additional instructions using conda env update --file environment.yml or conda env create -f environment.yml that would also install all optional packages but this would then require the user to get a modeller license and either setup their environment variables or modify the config.py license key file for modeller manually. It is far far from a simple one step process unless salilab have changed how they are distributing modeller since the last time I installed it.

[degiacom]

Good point. I personally like the idea of having a lean installation, especially considering that in any case the Modeller manual license makes a fully automated installation impossible anyway. We have updated the Installation section of the README files to make instructions clearer, hopefully this will be ok.

[degiacom]

@JoaoRodrigues, could you tell us if you are happy with the updates described above (for the JOSS publication https://github.com/openjournals/joss-reviews/issues/5523)? If so, happy for the issue to be closed?