icams / strucscan Goto Github PK

Joss Review

The code now runs for me on my computer and finishes, but I see a series of errors like the following:

>> collecting DUMMY/Al/total__fcc__Al ...
Problem in collecting 'total' properties from /.../strucscan//DUMMY/Al/total__fcc__Al:
line 31 in collector.py : <class 'RuntimeError'>
['  File "/.../strucscan/strucscan/core/collector.py", line '
 '31, in get_result_dict\n'
 '    result_dict = bulk.get_bulk_properties(calc, absolute_path)\n',
 '  File "/.../strucscan/strucscan/properties/bulk.py", line '
 '8, in get_bulk_properties\n'
 '    energy = final_struct.get_potential_energy()\n',
 '  File '
 '"/.../tmp-cbb0fa3f4c0dc28/lib/python3.10/site-packages/ase/atoms.py", '
 'line 726, in get_potential_energy\n'
 "    raise RuntimeError('Atoms object has no calculator.')\n"]
Atoms object has no calculator.

Is this an expected side-effect of running the dummy job? Or is this some error in strucscan itself? I find it hard to discern based on the error that is being output.

As part of the JOSS review, unit tests are required to demonstrate the functionality of the code:

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

While automated tests are not required, I would highly suggest implementing them using github actions as it will help with the long-term maintainability of the code. Similarly, I would recommend setting up a linter (but that's optional of course).

I see that there is something implemented in tests/, but it includes hard-coded paths, which are specific to the maintainer's system. These fail when I run it on my computer.

I am trying to test the functionality of your code as part of the JOSS Review, but I am running into trouble with running it. I get the following error:

» strucscan examples/dummy.yaml
Data tree path:                /home/mark/Downloads/strucscan/data
Structure repository:          /home/mark/Downloads/strucscan/structures
Resource repository:           /home/mark/Downloads/strucscan/resources

Optional key 'monitor' not provided. Default value will be used: True
Optional key 'submit' not provided. Default value will be used: True
Optional key 'collect' not provided. Default value will be used: True
Traceback (most recent call last):
  File "/home/mark/.virtualenvs/tmp-54286c793853f99/bin/strucscan", line 33, in <module>
    sys.exit(load_entry_point('strucscan', 'console_scripts', 'strucscan')())
  File "/home/mark/Downloads/strucscan/strucscan/cli.py", line 31, in main
    JobManager(input_dict)
  File "/home/mark/Downloads/strucscan/strucscan/core/jobmanager.py", line 118, in __init__
    if len(properties) > 1:
NameError: name 'properties' is not defined. Did you mean: 'property'?

I've tried a few different parameters in ~/.strucscan and the input file but to no avail. Any help would be appreciated.

As part of the review, I'm required to verify that the paper is well-written, and it is. However, the quality of writing in the documentation is a bit lower, with a fairly high number of typos and confusing sentences.

I started to list some typos, but decided instead to just fix them in a PR. I know I didn't find everything, so please review the documentation and fix anything I've missed.

JOSS Review

I see that almost all of the implemented methods include docstrings, but the API documentation does not seem to contain the same information and is a bit lacking. It should be straightforward to update the API documentation with the implemented docstrings and sphinx-apidoc.

Part of the JOSS review. These comments refer to the "Getting Started" part of the docs, particularly this section:

This exmaple illustrates a sample workflow using the pre-implemented dummy engine. To make use of it please configure the .strucscan configuration file and copy it in you home directory. You can use the template that comes by default with the repository and link the absoulte pathes to the structure and resource repository to the default ones from the repository.

! cat ../.strucscan
PROJECT_PATH: "data" # corresponds to the top node of your data tree
STRUCTURES_PATH: "structures"
RESOURCE_PATH: "resources"

DEBUG: FALSE # Default: FALSE
STRUCT_FILE_FORMAT: cfg # Default: cfg
SLEEP_TIME: 45 # Default: 45
In general, you might want to adapt the .strucscan to your own structure and resource directory and set up a resource directory accordingly. For now, you do not need to worry about it and we will discuss it later.

It's likely I haven't understood everything perfectly, but to run the Dummy example I needed to put this file in my home directory and update the PROJECT_PATH, STRUCTURES_PATH, and RESOURCE_PATH lines. The docs do say this, but I found the instructions a bit confusing and it took me several tries to get it right.

Some questions/requests:

• Is a global configuration file the only way to provide these settings? It seems that would prevent simultaneous calls to strucscan for separate projects. But maybe I'm missing something.
• I think it would be useful at this stage to provide short explanations for what is contained in STRUCTURES and RESOURCES. It seems if I were getting started, the first thing I would need to do is create these directories and populate them for my needs, but that's not immediately clear from the Getting Started docs. (The README does have some of this information.)
• Why are the structures and resources in the top-level repo? Would it be more clear to have them inside examples? But again, it feels like I may be missing something - so a bit more clarity in the Getting Started section would be useful.