`seekpath_structure_analysis`: `HubbardStructureData` compatibility

[mbercx]

Fixes #999

This PR was split off from https://github.com/aiidateam/aiida-quantumespresso/pull/998. Look there for the initial discussion on the choices made in this PR.

[mbercx]

Alright, started working on this, but will wait until after https://github.com/aiidateam/aiida-quantumespresso/pull/998 is merged so I can also refactor the structure fixtures a bit.

One thing to note is that the get_explicit_kpoints_path function also returns a conv_structure. But it seems that in the example test case I have it is the same as the primitive one, but different than the original which was constructed from a conventional FCC structure... 🤔 I'm probably too tired to understand this atm. ^^

[mbercx]

@bastonero in this comment you mention moving the logic for finding the primitive HubbardStructureData to HubbardUtils. A couple of thoughts here:

1. If I understand correctly, the purpose of the HubbardUtils class was to move QE-specific logic out of the HubbardStructureData class:

   https://github.com/aiidateam/aiida-quantumespresso/blob/d645069d1d6ac40d6a23e4bbf49b01b51f5bb33c/src/aiida_quantumespresso/utils/hubbard.py#L27-L28

   But I suppose the logic for finding the primitive HubbardStructureData would be more generic, so perhaps we can add a method to HubbardStructureData instead?

2. In the same vein, does the get_hubbard_for_supercell method contain any QE-specific logic?

3. My first inclination would be to create a HubbardStructureData.get_primitive method or something similar in this case, but I'm not sure I like this approach. In the end the StructureData also doesn't have anything similar, we'd pass it into the seekpath_structure_analysis calculation function to also track the provenance.

So I'm inclined to keep the logic where it is for now? Especially since it'll be rather incomplete. Once we have implemented the holy grail HubbardStructureData.apply_hubbard_to_structure() method that works in all cases (if this is possible ^^), we can still adapt.

[bastonero]

2. In the same vein, does the get_hubbard_for_supercell method contain any QE-specific logic?

I don't think it has any QE related logic, as it should be just a mapping.

3. My first inclination would be to create a HubbardStructureData.get_primitive method or something similar in this case, but I'm not sure I like this approach. In the end the StructureData also doesn't have anything similar, we'd pass it into the seekpath_structure_analysis calculation function to also track the provenance.

Fine to me to keep it there for the moment being,

So I'm inclined to keep the logic where it is for now? Especially since it'll be rather incomplete. Once we have implemented the holy grail HubbardStructureData.apply_hubbard_to_structure() method that works in all cases (if this is possible ^^), we can still adapt.

I think it's possible to have a generic apply_hubbard_to_structure. One has to play around, but one consideration is that the structures should be "commensurate". With this I mean that the atoms should not be rotated, the only thing that rotates are the cell vectors (this means the Cartesian coordinates remain the same, i.e. same reference system). So it involves a bit of thinking and time to implement it properly. Next time (:

[mbercx]

@bastonero @AndresOrtegaGuerrero I unfortunately don't have time to refactor the fixtures for generating structures, so the current test generates its own specific Hubbard structure.

I still don't really understand why the conv_structure is the same tbh.

[AndresOrtegaGuerrero]

I still don't really understand why the conv_structure is the same tbh.

What do you mean by this ? (You mean why seekpath returns a different structure as conv_structure from the input structure you provide ?) , Because, i feel this is an issue of seekpath and spglib

[mbercx]

What do you mean by this ? (You mean why seekpath returns a different structure as conv_structure from the input structure you provide ?)

That, and why the returned primitive and conventional structures are the same.

[AndresOrtegaGuerrero]

What do you mean by this ? (You mean why seekpath returns a different structure as conv_structure from the input structure you provide ?)

That, and why the returned primitive and conventional structures are the same.

Maybe we should open an issue an pin Giovanni for this ?

[mbercx]

@bastonero also added a test to check that structures with intersite parameters indeed raise a NotImplementedError.

[mbercx]

Thanks @bastonero. You mean an example where this fails (EDIT: i.e. with intersite interactions)? The current test_seekpath_analysis test already constructs a conventional Co structure with 4 sites and two kinds where the primitive structure only has two sites.

[bastonero]

Ann sorry @mbercx , didn't notice it was 4 sites. Good to go then!