fixed reading geometries from fdf + XV + STRUCT #779
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -10,7 +10,7 @@ | |||
| import pytest | |||
|
|
|||
| import sisl | |||
| from sisl import Atom, Geometry, geom | |||
| from sisl import Atom, AtomUnknown, Geometry, geom | |||
Check notice
Code scanning / CodeQL
Unused import Note test
|
This fails for me on the same files as reported in #778. With g0 = sisl.get_sile("./RUN.fdf").read_geometry(output=True)
print(g0.atoms.atom)I get this error message: |
|
Could you share your files? |
Ok, this is actually one of the weird cases where you likely have two different |
|
I think I know how to amend, let me give it a try! |
|
I have something like this: |
The different geometries reading mechanisms where kind-of broken for various constalletations in the XV+fdf+struct files. The species_as_Z was basicaly never going to works since there was no mechanism to counter it. Instead the XV and STRUCT siles now handle basis from the perspective of being passed an atoms argument, from which it will fill in stuff based on data. If the atoms is a *pure* basis object (only correct species indices). Then it will replace without checking stuff. If it has the same length as the geometry it will check Z and whether indices are the same. This *can* potentially lead to errors if there is only 1 atom of each species, but on the other hand the species order should then handle the indices correctly. There was also cases in fdf, XV and STRUCT reads that could potentially lead to reducing the basis. We should generally try to avoid this since there may be good reasons for having *empty* basis. Signed-off-by: Nick Papior <[email protected]>
Signed-off-by: Nick Papior <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
+ Coverage 87.32% 87.36% +0.03%
==========================================
Files 396 396
Lines 50592 50700 +108
==========================================
+ Hits 44178 44292 +114
+ Misses 6414 6408 -6 ☔ View full report in Codecov by Sentry. |
|
69d0f3f works correctly for my system! |
|
Here this doesn't work: fdf: XV: Which gives the following Z after reading with sisl: For some reason it works if the basis is in a |
|
Even just reading the geometry from the fdf file (without any basis) is wrong: import sisl
with open("geom.fdf", "w") as f:
f.write("""
NumberOfAtoms 2
NumberOfSpecies 2
%block ChemicalSpeciesLabel
1 79 Au
2 8 O
%endblock ChemicalSpeciesLabel
LatticeConstant 1.000 Ang
%block LatticeVectors
4 0 0
0 10 0
0 0 10
%endblock LatticeVectors
AtomicCoordinatesFormat Ang
%block AtomicCoordinatesAndAtomicSpecies
0 0 0 2
2 0 0 1
%endblock AtomicCoordinatesAndAtomicSpecies
""")
sisl.get_sile("geom.fdf").read_geometry().atoms.Z |
|
Thanks, my tests are obviously only testing species, not z... Damn... I'll have another stab at this! |
|
Actually, your test was exactly the kind of geometries that were the bottleneck, it would have worked if there were more atoms than species ;) But, I will fix this! |
|
Hmm I have another one that fails and has 260 atoms and 5 species. I thought this was a minimal example of it, but let's see 😅 Actually if you add another oxygen to the example that I provided it is also wrong 🤔 |
Signed-off-by: Nick Papior <[email protected]>
|
I added your Au, C example as a test, it passed, could you try the latest commit out? |
Signed-off-by: Nick Papior <[email protected]>
|
I'll just merge, it seems quite important ;) |
|
This example (with the ion files) still doesn't work 😅 |
|
Omfg! Could you share it? :) |
|
Was it this? #779 (comment) |
|
I'll see if I get time tomorrow... Sorry! |
|
Yes sorry, forgot to link it. There the species are still reversed. And if I add one extra oxygen I get: I don't understand why this simple case (reading XV from a fdfSileSiesta) should substitute the basis by checking things. The fdf contains a dictionary (or list) of species: and the first column of the XV refers explicitly to the key/index of the species as defined in |
But it seems you have some extra files, I would guess it actually reads the basis from somewhere else, I added the test for your case, and it passes fine, see sisl/src/sisl/io/siesta/tests/test_fdf.py Line 686 in 58f4758 |
|
Yes, #779 (comment) contains the XV and the ion files (the test is only the FDF) |
Thanks! I had missed that ;) It should be fixed in the latest commit! A problem only occurring when reading basis from ion files... :sigh: |
|
Btw, now the tests are full of warnings about replacing atoms 😅 |
|
Should be fixed now! :) |
The different geometries reading mechanisms
where kind-of broken for various constalletations
in the XV+fdf+struct files.
The species_as_Z was basicaly never going to
works since there was no mechanism to counter
it.
Instead the XV and STRUCT siles now handle
basis from the perspective of being passed
an atoms argument, from which it will
fill in stuff based on data.
If the atoms is a pure basis object (only
correct species indices).
Then it will replace without checking stuff.
If it has the same length as the geometry
it will check Z and whether indices are
the same. This can potentially lead
to errors if there is only 1 atom of each
species, but on the other hand the species
order should then handle the indices correctly.
There was also cases in fdf, XV and STRUCT reads
that could potentially lead to reducing the basis. We should generally try to avoid this since
there may be good reasons for having empty basis.
isort .andblack .[24.2.0] at top-leveldocs/CHANGELOG.mdReplaces #778