Handle space/point-group correctly

[edan-bainglass]

This PR uses pymatgen symmetry analyzer classes to extract symmetry group information from the structure for use in the summary.

Resolves #1053

[AndresOrtegaGuerrero]

@edan-bainglass Thank you! I think there should be a fallback for molecules as well, so it might be best to omit printing any symmetry for them. I'm not sure if pymatgen (when treating the structure as a molecule) provides point group information, or if this could be handled using a package like libmsym.

[AndresOrtegaGuerrero]

btw, i think also if you call the object , _StructureDataBaseViewer.cell_spacegroup.value and _StructureDataBaseViewer.cell_hall.value you get the infor right ?

[edan-bainglass]

@AndresOrtegaGuerrero can you recheck please?

[edan-bainglass]

btw, i think also if you call the object , _StructureDataBaseViewer.cell_spacegroup.value and _StructureDataBaseViewer.cell_hall.value you get the infor right ?

I try to avoid model logic relying on the UI. This breaks model independence.

[AndresOrtegaGuerrero]

This is my test with a 2D structure,

I would suggest add a test using a 2d or 1d strucutre for the summary.

I will check with a molecule , i have a feeling that with a molecule you might encounter issues since you are using spglib, so the cell dimensions matter when computing the point group, so i will do a test with methane and change the cell.

[AndresOrtegaGuerrero]

The test for the molecule ,

If you notice, you're printing the space group as the point group (and the "P" is missing). The space group you're displaying is derived from the cell operations (spglib). Since this is methane, the correct point group should be T_d.

Also, note how the space group changes when the cell parameters are modified.
for this system you will be printing a different Point group

[AndresOrtegaGuerrero]

One solution , i am not sure, is to make a temporary Molecule object from pymatgen , https://pymatgen.org/pymatgen.symmetry.html , and i think like that you might get the point group , and then use this class, class PointGroupAnalyzer

[AndresOrtegaGuerrero]

@edan-bainglass this code works for the molecules

	from pymatgen.symmetry.analyzer import PointGroupAnalyzer

	molecule_aiida
	molecule_pymatgen = molecule_aiida.get_pymatgen_molecule()
	analyzer = PointGroupAnalyzer(molecule_pymatgen)
	point_group = analyzer.get_pointgroup() 

I tested with methane and it return

In [5]: analyzer.get_pointgroup() Out[5]: Td

[cpignedoli]

@AndresOrtegaGuerrero, I'm not sure I can comment, you collected more statistics than I did.

[edan-bainglass]

@edan-bainglass this code works for the molecules

	from pymatgen.symmetry.analyzer import PointGroupAnalyzer

	molecule_aiida
	molecule_pymatgen = molecule_aiida.get_pymatgen_molecule()
	analyzer = PointGroupAnalyzer(molecule_pymatgen)
	point_group = analyzer.get_pointgroup() 

I tested with methane and it return

In [5]: analyzer.get_pointgroup() Out[5]: Td

I'm happy to adjust to whatever you suggest here, as you clearly have more experience with this 😅 I will give this a try.

[AndresOrtegaGuerrero]

@edan-bainglass this code works for the molecules

	from pymatgen.symmetry.analyzer import PointGroupAnalyzer

	molecule_aiida
	molecule_pymatgen = molecule_aiida.get_pymatgen_molecule()
	analyzer = PointGroupAnalyzer(molecule_pymatgen)
	point_group = analyzer.get_pointgroup() 

I tested with methane and it return
In [5]: analyzer.get_pointgroup() Out[5]: Td

I'm happy to adjust to whatever you suggest here, as you clearly have more experience with this 😅 I will give this a try.

This is only needed for the molecules, the rest of the code with spglib is fine

[edan-bainglass]

@edan-bainglass this code works for the molecules

	from pymatgen.symmetry.analyzer import PointGroupAnalyzer

	molecule_aiida
	molecule_pymatgen = molecule_aiida.get_pymatgen_molecule()
	analyzer = PointGroupAnalyzer(molecule_pymatgen)
	point_group = analyzer.get_pointgroup() 

I tested with methane and it return
In [5]: analyzer.get_pointgroup() Out[5]: Td

I'm happy to adjust to whatever you suggest here, as you clearly have more experience with this 😅 I will give this a try.

This is only needed for the molecules, the rest of the code with spglib is fine

By the way, what is molecule_aiida? That line is just floating there 😅

[edan-bainglass]

@AndresOrtegaGuerrero, also, since we are now suggesting to use pymatgen for a molecule, was the previous use of pymatgen's analyzer for space groups wrong in principal, or can we fall back on it for non-molecule systems?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.99%. Comparing base (1df8f54) to head (1927b0b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
+ Coverage   67.83%   67.99%   +0.16%     
==========================================
  Files         112      112              
  Lines        6613     6634      +21     
==========================================
+ Hits         4486     4511      +25     
+ Misses       2127     2123       -4     

Flag	Coverage Δ
python-3.11	67.99% <100.00%> (+0.16%)	⬆️
python-3.9	68.01% <100.00%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AndresOrtegaGuerrero]

the previous one (crystals) will fail for 1d, and 2d periodicity since the structure.pbc is different than (True, True, True)

[edan-bainglass]

the previous one (crystals) will fail for 1d, and 2d periodicity since the structure.pbc is different than (True, True, True)

@AndresOrtegaGuerrero please review the latest changes. We may need to zoom to address this.

[AndresOrtegaGuerrero]

I tested the PR , for molecules it works,
The molecule is methane , Point group Td

For 2D you get this error,

~/apps/quantum-espresso/src/aiidalab_qe/app/result/components/summary/model.py in generate_report_html(self)
     70         env = Environment()
     71         template = files(templates).joinpath("workflow_summary.jinja").read_text()
---> 72         parameters = self._generate_report_parameters()
     73         report = {key: value for key, value in parameters.items() if value is not None}
     74         schema = json.load(Path(__file__).parent.joinpath("schema.json").open())

~/apps/quantum-espresso/src/aiidalab_qe/app/result/components/summary/model.py in _generate_report_parameters(self)
    189         }
    190 
--> 191         symmetry_group_info = self._get_symmetry_group_info(structure)
    192         report["initial_structure_properties"] |= symmetry_group_info
    193 

~/apps/quantum-espresso/src/aiidalab_qe/app/result/components/summary/model.py in _get_symmetry_group_info(self, structure)
    289         pymatgen_structure = structure.get_pymatgen()
    290         if any(structure.pbc):
--> 291             analyzer = SpacegroupAnalyzer(structure=pymatgen_structure)
    292             symbol = analyzer.get_space_group_symbol()
    293             number = analyzer.get_space_group_number()

~/.local/lib/python3.9/site-packages/pymatgen/symmetry/analyzer.py in __init__(self, structure, symprec, angle_tolerance)
    115         else:  # if no magmoms given do not add to cell
    116             self._cell = (
--> 117                 tuple(map(tuple, structure.lattice.matrix.tolist())),
    118                 tuple(map(tuple, structure.frac_coords.tolist())),
    119                 tuple(zs),

AttributeError: 'Molecule' object has no attribute 'lattice'

An the issue is because when you do pymatgen_structure = structure.get_pymatgen() , you are getting a object from the class Molecule and not a structure, and the class SpacegroupAnalyzer( https://pymatgen.org/pymatgen.symmetry.html#pymatgen.symmetry.analyzer.SpacegroupAnalyzer) is expecting an object Structure

The reason you get a Molecule object is because your aiida StructureData has a periodicity (True, True, False) , (You will have the same issue for 1d (True, False, False)

[AndresOrtegaGuerrero]

maybe you can do something like ,

structure_temp = self.aiida_structure.clone()
structure_temp.pbc = [True, True, True]
structure = structure_temp.get_pymatgen()

or use the code you had before using ase2splib

[AndresOrtegaGuerrero]

@edan-bainglass this code works for the molecules

	from pymatgen.symmetry.analyzer import PointGroupAnalyzer

	molecule_aiida
	molecule_pymatgen = molecule_aiida.get_pymatgen_molecule()
	analyzer = PointGroupAnalyzer(molecule_pymatgen)
	point_group = analyzer.get_pointgroup() 

I tested with methane and it return
In [5]: analyzer.get_pointgroup() Out[5]: Td

I'm happy to adjust to whatever you suggest here, as you clearly have more experience with this 😅 I will give this a try.

This is only needed for the molecules, the rest of the code with spglib is fine

By the way, what is molecule_aiida? That line is just floating there 😅

Just your object StructureData from aiida, that has a molecule , so the pbc = (False,False,False)

[edan-bainglass]

@AndresOrtegaGuerrero am I misunderstanding the AiiDA API then?

structure.get_pymatgen() calls

    def get_pymatgen(self, **kwargs):
        """Get pymatgen object. Returns pymatgen Structure for structures with periodic boundary conditions
        (in 1D, 2D, 3D) and Molecule otherwise.
        :param add_spin: True to add the spins to the pymatgen structure.
        Default is False (no spin added).

        .. note:: The spins are set according to the following rule:

            * if the kind name ends with 1 -> spin=+1

            * if the kind name ends with 2 -> spin=-1

        .. note:: Requires the pymatgen module (version >= 3.0.13, usage
            of earlier versions may cause errors).
        """
        return self._get_object_pymatgen(**kwargs)

which calls

    def _get_object_pymatgen(self, **kwargs):
        """Converts
        :py:class:`StructureData <aiida.orm.nodes.data.structure.StructureData>`
        to pymatgen object

        :return: a pymatgen Structure for structures with periodic boundary
            conditions (in three dimensions) and Molecule otherwise

        .. note:: Requires the pymatgen module (version >= 3.0.13, usage
            of earlier versions may cause errors).
        """
        if any(self.pbc):
            return self._get_object_pymatgen_structure(**kwargs)

        return self._get_object_pymatgen_molecule(**kwargs)

So for anything not 0D (molecule), you should have some PBC (in other words, any PBC), and thus should get a Structure. But somehow your 2D system is getting a Molecule?

[AndresOrtegaGuerrero]

My test was conducted using AiiDA 2.5.1, so the behavior might differ in newer versions. Try simulating a 2D system and check the results. If the behavior doesn't occur in the latest AiiDA, it could indicate an incompatibility with version 2.5.1.

[edan-bainglass]

@AndresOrtegaGuerrero as discussed, the StructureData.get_pymatgen method was changed recently to include 1D and 2D systems as structures instead of molecules. For now, my fix does not rely on the method directly, but instead calls directly the structure/molecule flavors of get_pymatgen (which are called by it anyhow). I will open an issue stating that this workaround should fall back on get_pymatgen once AiiDAlab is updated with newer AiiDA throughout.

[AndresOrtegaGuerrero]

LGTM! Thank you @edan-bainglass I tested with a molecule and with a 2D system