JOSS review

[TomTranter]

Hi @lbluque I'm in the process of reviewing smol for your JOSS submission. I will add things here when I find them.

• Installation on windows: After trying to install with "pip install smol" as recommended I received and error for one of the dependencies spglib. I then used conda to install this package from conda-forge instead. The error concerned not having a version of Microsoft Visual C++ even though I do have this. Have you tested the code on Windows? Is this a known issue with spglib?

[lbluque]

Hi @TomTranter Thanks a lot for getting this started!

Currently we are not running CI tests for windows. However, I have personally tried to install and use the package on windows without issue.

The spglib dependency comes from pymatgen. I was able to find this discussion thread, which I think is a similar issue that you had. The suggested workaround is to install pymatgen from conda-forge (which is conceptually the same solution you used). But unfortunately I don't think I can address that directly here using pip. The "workaround" I can think of is to add a comment describing this in the installation steps.

[TomTranter]

Hi @lbluque so actually conda-forge installing just spglib didn't work and you are right I had to install pymatgen, you could introduce a environment.yml for conda installers.

[TomTranter]

I am now trying to run the examples found here https://cedergrouphub.github.io/smol/notebooks/creating-a-ce.html but the files needed are not to be found in the package so I must clone the repository. To make running the examples easier you could provide a collab workspace and make sure they are all runnable with the files in the repository not just the docs

[TomTranter]

@lbluque the summary in the paper needs to be improved to explain a little more about cluster expansion for a non-specialist

[TomTranter]

@lbluque the visualisation example contains no plots. https://cedergrouphub.github.io/smol/notebooks/cluster-visualization.html
The Crystal Toolkit Jupyter Lab is not installed on the document rendering website I guess... and I also cannot install it locally to run the example. I'm getting errors on windows with the pip install. It's a shame because I feel the package would really benefit from some visualisation. Perhaps a more basic matplotlib function should be provided as a fall back when this package is not installed. The arrays are hard to interpret by themselves

[TomTranter]

@openjournals/joss-reviews#4504 I have conducted the review and made some suggestions above

[lbluque]

👋 @TomTranter thanks so much for taking the time and the detailed suggestions!

I'll mention them individually as I address them.

[lbluque]

Hi @lbluque so actually conda-forge installing just spglib didn't work and you are right I had to install pymatgen, you could introduce a environment.yml for conda installers.

Glad to hear installing pymatgen from conda-forge solved the issue. I added additional instructions to the README file and the documentation.

I also added an environment.yml file for user convenience when using conda.

[lbluque]

I am now trying to run the examples found here https://cedergrouphub.github.io/smol/notebooks/creating-a-ce.html but the files needed are not to be found in the package so I must clone the repository. To make running the examples easier you could provide a collab workspace and make sure they are all runnable with the files in the repository not just the docs

I created a binder link to be able to run the notebooks online. I added a badge and instructions in the README and documentation.

[lbluque]

@lbluque the visualisation example contains no plots. https://cedergrouphub.github.io/smol/notebooks/cluster-visualization.html The Crystal Toolkit Jupyter Lab is not installed on the document rendering website I guess... and I also cannot install it locally to run the example. I'm getting errors on windows with the pip install. It's a shame because I feel the package would really benefit from some visualisation. Perhaps a more basic matplotlib function should be provided as a fall back when this package is not installed. The arrays are hard to interpret by themselves

It is a shame that you were unable to get crystal-toolkit running because it is a great package, and the crystal visualizations look great with it. You should be able to see these and test it out in the Binder I just added.

I also added an example of how to use Plotly (since it is interactive) to visualize a cluster in-lieu of crystal-toolkit. I am hesitant to add this type of functionality directly in our code, since it essentially recreates functionality that already exists within the Materials Project ecosystem that our code is built upon.

I'm afraid I also can't do much in terms of the visualizations showing up in the statically rendering of jupyter right now, since Crystal-Toolkit does not support this yet. See this comment:
#201 (comment)

[lbluque]

@lbluque the summary in the paper needs to be improved to explain a little more about cluster expansion for a non-specialist

I added this explanation to the summary:
"The cluster expansion method is a formal and effective way to construct functions of atomic configuration by coarse-graining materials properties, such as formation energies, in terms of species site occupations in a lattice model [Sanchez:1984]."

[lbluque]

@TomTranter I think I addressed all your suggestions to some extent now. Please have a look at them when you have some time, and thanks again for taking the time to review our code!

[TomTranter]

@lbluque the visualisation example contains no plots. https://cedergrouphub.github.io/smol/notebooks/cluster-visualization.html The Crystal Toolkit Jupyter Lab is not installed on the document rendering website I guess... and I also cannot install it locally to run the example. I'm getting errors on windows with the pip install. It's a shame because I feel the package would really benefit from some visualisation. Perhaps a more basic matplotlib function should be provided as a fall back when this package is not installed. The arrays are hard to interpret by themselves

@lbluque any chance you can fix this visualisation? I think that's it really and everything else looks good to me

[lbluque]

Thanks for the quick turnaround. The example using plotly was not showing up in the documentation because it had not been/built and deployed. I did a manual build and it should show up now.

Unfortunately the crystal-toolkit visualizations cant show up in the documentation since the crystal-toolkit does not support this yet. I personally would very much like those visualizations to show up, so at some point I may PR the necessary code to the crystal-toolkit, but I am not very well versed in web development/dash/js etc so I won't be able to do it myself anytime soon.

The crystal-toolkit visualizations do work properly in the interactive Binder. notebook.

[TomTranter]

Ok all looks great thanks