seekpath_structure_analysis: HubbardStructureData compatibility

[mbercx]

Fixes #999

This PR was split off from #998. Look there for the initial discussion on the choices made in this PR.

[AndresOrtegaGuerrero]

Here we should add the NotImplementedError with a clear message to the user or implement a generalized function(s) for V

[mbercx]

I know. ^^ Just for future reference: I typically leave a PR in "Draft" status until it's ready for proper review, and request a review at that point. 🙃

[AndresOrtegaGuerrero]

ah yeah i know , i was putting a comment here just as a reminder , sorry :)

[mbercx]

Alright, started working on this, but will wait until after #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:

   aiida-quantumespresso/src/aiida_quantumespresso/utils/hubbard.py

   Lines 27 to 28 in d645069

   	class HubbardUtils:
   	"""Utility class for handling `HubbardStructureData` for QuantumESPRESSO."""

   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 ?

[AndresOrtegaGuerrero]

LGTM

[bastonero]

Thanks @mbercx. Maybe not in this PR, but we should block at the very beginning the usage of HubbardStructureData with intersites in the Bands workchain.

Just left a minor change request, then looks good.

[bastonero]

One can still have intersite parameters, but indicate U for instance. Why not using the utils.hubbard.is_intersite_hubbard method?

[mbercx]

Good point, thanks @bastonero. The is_intersite_hubbard function is perfect indeed.

[mbercx]

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

[bastonero]

Thanks a lot @mbercx, great job. Sorry to be a bit paranoid, but it would be good to have a test where the primitive structure is smaller then the original one. For instance, cubic Si? The input structure being 8-atoms, it should return the primitive structure with 2 atoms. Or you can also simply make a supercell using from ase.build import make_supercell if I remember correctly.

[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!

[bastonero]

@mbercx thanks a lot, good for me!