Adding jarvis-tools structures

[knc6]

No description provided.

[knc6]

Hi @CasperWA and @ml-evs ,

All tests pass on my personal computer, but in the ci, probably because it didn't install the jarvis-tools.
I am not sure if changes in the docker container would be required. Could you take a look?

Best,
Kamal

[CasperWA]

Great! Thanks @knc6!

I have a few comments and suggested changes.

Also, I think I found the issue of why our CI tests are failing, and I also have a question regarding the data structure of jarvis.tools.Atoms:elements.

[CasperWA]

All tests pass on my personal computer, but in the ci, probably because it didn't install the jarvis-tools.
I am not sure if changes in the docker container would be required. Could you take a look?

Our CI tests that the expected response is correct for the cases where the required adapter packages are and are not installed. It fails for a test that checks the expected response if the package requirement is not installed (see my comments in my review).

[codecov]

Codecov Report

Merging #297 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #297      +/-   ##
==========================================
+ Coverage   90.04%   90.12%   +0.08%     
==========================================
  Files          54       55       +1     
  Lines        2270     2289      +19     
==========================================
+ Hits         2044     2063      +19     
  Misses        226      226              

Flag	Coverage Δ
#unittests	90.12% <100.00%> (+0.08%)	⬆️

Impacted Files	Coverage Δ
optimade/adapters/structures/jarvis.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1cac1e...38ac621. Read the comment docs.

[knc6]

Thanks @CasperWA for the feedback. I think I have taken all of them into account.

[knc6]

About your comment on the Atoms object in the jarvis-tools, the elements is a character array such as ['Ac','Ac']. They can be converted to 'jarvis.core.specie.Specie' object for obtaining properties such as atomic mass, atomic number etc. @CasperWA

[CasperWA]

About your comment on the Atoms object in the jarvis-tools, the elements is a character array such as ['Ac','Ac']. They can be converted to 'jarvis.core.specie.Specie' object for obtaining properties such as atomic mass, atomic number etc.

Right, so it can also deal with:

[
  {
    "name": "OX2",
    "chemical_symbols": ["O", "vacancy"],
    "concentration": [0.7, 0.3],
    "mass": 16.0,
    "original_name": "O1"
  },
  {
    "name": "Cu",
    "chemical_symbols": ["Cu"],
    "concentration": [1.0],
    "mass": 63.5,
    "original_name": "Cu-species"
  }
]

Which is what the value of attributes.species may look like.

[knc6]

Yes, I think so too. But we do not have the vacancy in the Specie object. We mention in the 'test_special_species' function. We define a separate Vacancy object.

[knc6]

The requested change doesn't load, not sure why.

[CasperWA]

The requested change doesn't load, not sure why.

Do you mean it doesn't show because I've "resolved" them? You should be able to just press them and see what they were about :)

By the way, I've rebased your commits on the latest commit in the master branch, as well as remove some unused imports in the test file.

[CasperWA]

Yes, I think so too. But we do not have the vacancy in the Specie object. We mention in the 'test_special_species' function. We define a separate Vacancy object.

So the chemical_symbols is a list of chemical elements (case sensitive), but can also include "X" as an unknown element, which isn't a vacancy, and then of course also "vacancy", which will represent a vacancy.
If jarvis.core.atoms.Atoms cannot handle these ("X"), you should try to handle that before returning the Atoms object.
For "vacancy" you're already doing this, since you're checking for "disorder" and raising if found in structure_features, so that's excellent! :) However, a structure may also have a single species with chemical_symbols = ["X"], which isn't caught by this check. Would this break jarvis' Atoms?

Actually, we are not properly testing for this at the moment! So I think I'll create a separate PR adding this test.
For this PR, I consider it good-enough at this time, but the adapter may have to be updated for the separate PR.
If you want to include the "fix" already in this PR that's great! :) Otherwise I would consider it fine to add it in the separate PR, including the other fixes that would need to be done (for, e.g., ASE).

[CasperWA]

@knc6 I've been looking a bit into the code of jarvis, and it seems the Specie class you're wrapping the entries of Atoms.elements in takes a str as initializer.
I would suggest to then initialize your Atoms instance in the adapter, providing elements with a generated list of OPTIMADE species' name key, i.e.:

Atoms(
    elements=[specie.name for specie in attributes.species],
    # ... other instantiating attributes ...
)

Do you think this makes sense? Instead of your Specie class having to initalize with symbol being a Python dictionary instead of a string (which would currently be the case)?

[CasperWA]

@knc6 I have updated your test here to take into account structures with special non-disordered species (i.e., updating it according to the newly added tests in #305.
I hope it's all right? If not, please make the appropriate changes on top.
If you're having trouble getting a local version matching the one here on github, you can first do git fetch --all -p followed by (as long as you have checked out this branch locally) git reset --hard knc6/master. This will reset your local version of the branch to the one on GitHub disregarding any differences.

[knc6]

@CasperWA I changed the jarvis.py in the adapters/structures accordingly. Your changes makes sense to me. Atoms constructor should be able to take 'X' or any other character. It will only fail, if we ask for some chemical properties such as Specie('X').atomic_mass.

[CasperWA]

Thanks for this addition @knc6 !

I think all is as it should be.