alejandrojuria / tightbinder Goto Github PK

One of the tests, test_conductance is failing.

>       assert np.allclose(conductance, 6.064302600409135)
E       assert False
E        +  where False = <function allclose at 0x1027c3d30>(6.240297897758343, 6.064302600409135)
E        +    where <function allclose at 0x1027c3d30> = np.allclose

tests/test_observables.py:147: AssertionError

@alejandrojuria

I am one of the reviewers for your JOSS paper. I'll open separate issues for each of the comments that I want to be addressed.

setup.py is deprecated and the new installation approach uses pyproject.toml. Your installation is simple enough that conversion shouldn't take much time after you understand the new build framework. Please replace setup.py with pyproject.toml.

I don't have to install the dependencies such as numpy, scipy, and matplotlib separately. You can specify them within the setup.py or pyproject.toml, so they are installed automatically along with tightbinder.

After #6 is addressed, please specify the dependencies within the pyproject.toml

I recommend the following layout to prevent confusion

examples/*.txt -> examples/inputs/*.yaml (After you convert them to yaml)
scripts/examples/*.py -> examples/*.py

I looked at the input file format and the code used to parse the file. Both are, to be honest, not well implemented. Here are some thoughts on this.

1. As the other reviewer mentioned, you could have used well-known formats such as ASE, pymatgen or VASP formats for the input crystal structure.
2. The species type is missing in all the example input files.
3. If you don't want to introduce big packages such as pymatgen as dependencies, the input file is essentially a dictionary/map. You could have used YAML or JSON for this. Those formats give you all the flexibility and you don't have write custom code to parse the text. For example GaSe.txt could be GaSe.yaml as follows

SystemName: GaSe
Dimensions: 2
Lattice: 
  - 3.30735 -1.90950  0.00000
  - 0.00000  3.81900  0.00000
Species: [Ga, Se] # Instead of 2, I am putting the species types
Motif: [
  -1.102  -1.910  1.235  0
  -1.102  -1.909 -1.235  0
  -2.205  -0.000  2.409  1
  ]
Filling: [0.5, 0.5]
Orbitals: [pz, pz]
OnsiteEnergy: [0.41, -0.41]
# OnSiteEnergyUnit, if you want to specify different units
SKAmplitudes: 
  - [0, 1; 1] 0 -2.33 # This line may not be parsed properly by yaml, but could be easily reformatted for yaml
  - [0, 1; 2] 0 0.61
  - [0, 0; 3] 0 0.07
  - [1, 1, 3] 0 0.07
  - [0, 1; 4] 0 0.13
  - [0, 0; 5] 0 -0.09
  - [1, 1, 5] 0 -0.09
Spin: False
SOC: [0, 0]
Mesh: [100, 100]
CriticalPoints: M 

and the code to parse the file would be very simple as below.

import yaml

# Read YAML file
with open("GaSe.yaml") as fp:
    data = yaml.safe_load(fp)
    configuration = shape_arguments(data)

Hello @alejandrojuria , thank you for your excellent work on this code.
Here are some comments that you may consider to improve the quality of the submitted manuscript (openjournals/joss-reviews#5810) and the features of the code.

• The code, 'tightbinder', is reported to be an alternative tool that has some advantages over the popular tight binding code in the simulations of the realistic amorphous materials and the identification of the topological properties in the simulated systems. I didn't have experience with the other codes. Could you prepare a simple table or enumerated list to highlight the features of tightbinder that are not available or not easy to use in other codes? This highlight may be included in the README file. I know you have listed some functional features in the README, but let us know if these features are unique of tightbinder if you have already had the experience. I think it may help the upcoming new users to choose which codes they can take for their specific case.
• In the manuscript, when mentioning the two Python classes designed for crystalline and amorphous solids, authors may include the specific names of the two classes.
• On page 3 of the manuscript, change 'SK' to 'Slater-Koster' since it was only used once on line 83.
• On line 95 of page 3, the authors are suggested to provide the website of the documentation which can be easily accessed by the readers.
• I tested to build the documentation. I executed make html, but it raised the error: make: *** No rule to make target 'html'. Stop. make: *** No rule to make target 'html'. Stop.
  The "make'' refers to the version make-4.4.1 installed in my computer.
• In all the tutorials, all paths for loading the files omit a dot (all my tests were run in a Linux mint system). However, if the paths are only shown for demo, it's okay and you don't need to change it.
• In the tutorial for building the crystal https://tightbinder.readthedocs.io/en/latest/tutorial/crystal.html, execution of model.plot_wireframe() in Jupyter notebook raised an error TypeError: list indices must be integers or slices, not tuple'' that seems to come from ``File ~/tightbinder/tightbinder/system.py:520, in System.plot_wireframe(self, ax, linewidth, alpha, color)''.
• In the tutorial for setting up the device (https://tightbinder.readthedocs.io/en/latest/tutorial/transport.html), these example codes all raised parsing error ValueError: Filling must be a number. I think there could be some errors in the examples/chain.txt.
• Have you considered introducing ASE to this code (or Pymatgen or any other built-in methods in this code) so that the crystal can be built directly from the popular structure files in POSCAR and CIF? If it has been implemented, you're encouraged to mention it in the tutorial and the manuscript.
• Automated tests are still lacking to verify the core functionalities (you can find below that these tests are required by Review Checklist of JOSS)

Here, I also linked this issue to the Review checklist openjournals/joss-reviews#5810 (comment) so that you can track the status of this review report.

Tests are missing. For code quality control, please consider adding unit tests. Python unit testing framework should be enough for your case.