-
Notifications
You must be signed in to change notification settings - Fork 2
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] Comments #1
Comments
Hi @AntObi, thanks for taking such a detailed look. I have since given the documentation a nearly complete overhaul, and most what didn't work or was out of date was removed. The current documentation https://besmarts.readthedocs.io/en/latest/index.html should be up to date and all examples and code snippets should be working. I'll give a broken down response below: InstallationThe PyPI instructions were outdated and based on old software that this package has superseded. I have removed this reference for now to reflect the current state of the package, and will add it again once the package is up on PyPI. TestingThanks for catching this! I built the tests using the ExamplesThanks for giving these a spin. I've fixed the UsageImport errors and other bugs have been fixed. Some code which has been indicated here (in particular There is now an example showing how to label a molecule. Should be fixed and working now. Should be fixed and working now. Ah, good catch. I've since moved this functionally to a difference branch and forgot to remove this page. I plan to further work on this later since it is not essential for the main purpose of this package |
Hi @trevorgokey , I've taken another look at the repository and have some further issues I've spotted InstallationCurrently, the installation in a fresh environment fails for me, which I have identified the causes. For the setup.py for besmarts/besmarts-rdkit/python/setup.py Lines 17 to 22 in 93c9ac9
As well for besmarts/besmarts-scipy/python/setup.py Lines 18 to 22 in 93c9ac9
TestsCurrently 22/23 tests pass. The test besmarts/besmarts-core/python/tests/test_import.py Lines 10 to 34 in 93c9ac9
Examplesbesmarts-core/python/examples/My configuration for running these examples was as follows:
In besmarts-core/python/besmarts/mechanics/fits.py the following import statement exists
Example 4 is failing for me because of this line:
I will follow up by testing the examples in the documentation as well as elsewhere in the repository where I find them |
It turned out I had a lot of local files that were not commited to the repo so you got a lot of missing file issues. This should all be fixed. Thanks for your patience, persistence, and diligence here. |
Hi @trevorgokey , I'm using the issue tracker here to point out what I have found from my review openjournals/joss-reviews#6653 .
Installation
Installation instructions in the README of the repository work fine, but the installation instructions in the Readthedocs does not work. There are currently no [PyPI packages] (https://pypi.org/search/?q=besmarts) related to your packages.
Testing
I've ran the tests using
pytest
and they all 'pass'. However, I recommend that in line 34 oftest_branch_split.py
,besmarts/besmarts-core/python/tests/test_branch_split.py
Line 34 in 02a8c4c
test
to something that does not have test in its name as this causes pytest to assume that it is a function to be tested. You can see the output of running pytest in the expandable section below.Test output from running
pytest -v
Examples
besmarts-core/python/examples/
My configuration for running these examples was as follows:
bond_union.py
joss-manuscript
branch, but it is on themain
branch.forcefield_fit.py
besmarts-scipy
. Fails*matching.py
smarts_cluster_bond_lengths.py
smarts_split.py
smiles_io.py
*
forcefield_fit.py
andbesmarts-scipy/python/examples/optimize_positions.py
fail despitebesmarts-scipy
being installed with the following error:Usage - RTD
In the documentation I have found areas in which the examples do not work.
Graphs
from besmarts.codecs import native
does not work:ImportError: cannot import name 'native' from 'besmarts.codecs' (unknown location)
In the SMILES/SMARTS binary operations section, there appears to be syntax errors with lines ending in colons (:) such as
A, B = graphs.graph_to_structure_atoms(g)[:2]:
In the following code block, the variable
bonds
is used but is never defined, andstructure_mapper
does not appear to be a function in themapper
module from a search of the codebase:Labelling
Splitting
besmarts.splitter
does not exist, so the first example cannot be run on this pageClustering
smiles_assignment_str
is not imported prior to its useTypeError: cluster_means() missing 1 required positional argument: 'sag'
Datasets
besmarts.resolve
is not a module. I cannot run this example.Documentation
From a brief scan of the codebase, many of the classes and the functions require documentation to make it easy for an external user to make good use of the modules available in this package.
The text was updated successfully, but these errors were encountered: