[WIP] Cpp interface by LecrisUT · Pull Request #301 · spglib/spglib

LecrisUT · 2023-06-18T12:34:04Z

Having this in parallel helps me figure out how to build the test fixtures

codecov-commenter · 2023-06-18T13:04:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.59%. Comparing base (7e7fe81) to head (af390e4).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #301   +/-   ##
========================================
  Coverage    83.59%   83.59%           
========================================
  Files           24       24           
  Lines         8118     8118           
  Branches      1686     1693    +7     
========================================
  Hits          6786     6786           
  Misses        1332     1332

Flag	Coverage Δ
c_api	`74.43% <ø> (ø)`
fortran_api	`56.16% <ø> (ø)`
python_api	`79.70% <ø> (ø)`
unit_tests	`13.47% <ø> (ø)`

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.

LecrisUT

I am slowly moving forward with this. First step, let's discuss the data structure @lan496 @atztogo. After that I will add appropriate interfaces on top of that

LecrisUT · 2024-02-28T18:47:25Z

cpp/spglib.hpp

+	class Symmetry {
+		// TODO: Find better interface compatible with std::mdspan
+		std::array<double, 9> rotations;
+		std::array<double, 3> translations;
+	};
+	class MagneticSymmetry : public Symmetry {
+		bool time_reversal;
+	};


Should this be split into RotationSymmetry and TranslationSymmetry, or are there cases when both are simultaneously non-zero? Similarly with MagneticSymmetry -> TimeReversalSymmetry?

And there's also PointSymmetry that is a subset of these.

Time reversal operation (prime operation) commutes with space group operation, so this inheritance would work, but I am not very sure, and not also sure if this is intuitive. @lan496 can consider in depth on this.

If this class was used in implementing core algorithms, the split or reconsideration would be needed. But I think this is thin wrapper for C-API, so the current one looks good.

LecrisUT · 2024-02-28T18:51:12Z

cpp/spglib.hpp

+	class Spacegroup {
+		int number;
+		int hall_number;
+		std::string international_symbol;
+		std::string hall_symbol;
+		std::string schoenflies;
+		std::string pointgroup_international;
+		std::string pointgroup_schoenflies;
+		int arithmetic_crystal_class_number;
+		std::string arithmetic_crystal_class_symbol;
+		// TODO: Unclear what this is referencing
+		char choice[6];
+	};


Needs review if these are sufficient or overcomplete.

Space group and space-group-type have different meanings in crystallography. So here SpacegroupType is more appropriate.

Space group and space-group-type have different meanings in crystallography. So here SpacegroupType is more appropriate.

Good to know.

What about the contents of it? E.g. hall_number and hall_symbol should be linked right?

Do you ask about one-to-one correspondance between hall_number and hall_symbol?

Yes, among other things. What makes sense to group together, what is a different convention of another, etc. The info would still be accessible flat, e.g. soacegroup->hall_number(), but it should be stored in more organized containers.

Hall number is not a proper word in crystallography, but we in spglib use it as the serial ID of the Hall symbol.

LecrisUT · 2024-02-28T18:51:53Z

cpp/spglib.hpp

+	// TODO: should it inherit/reference Spacegroup
+	class MagneticSpacegroup {
+		int number;
+		int type;
+		int uni_number;
+		int litvin_number;
+		std::string bns_number;
+		std::string og_number;
+	};


Isn't the MagneticSpacegroup also a Spacegroup?

Magnetic space group and space group are different groups and corresponding objects live in different spaces.

Similar to space group, MagneticSpacegroupType is more appropriate.

But from what I see in SpglibMagneticDataset it also has a hall_number associated. So do all MagneticSpacegroupType have a SpacegroupType/hall_number/etc. associated with it?

Hall symbols are defined for space group type with selected crystallographic settings. A subset of each magnetic space group (MSG) forms a space group, and the Hall symbol of the space group is useful information in MSG. But we have to remember that the MSG and the subset that forms space group in general don't have group-subgroup relation.

Hmm, we could store it as a pointer on MSG, pointing to the SG

LecrisUT · 2024-02-28T18:53:15Z

cpp/spglib.hpp

+	// TODO: Combine/templetize
+	class Dataset {
+		std::vector<Symmetry> symmetries;
+		Spacegroup spacegroup;
+		// These are dependent on the input, should be moved to constructor?
+		std::array<double, 9> transformation_matrix;
+		std::array<double, 3> origin_shift;
+		// Smart reference to the input used
+		std::shared_ptr<Lattice> lattice;
+	};
+	class MagneticDataset {
+		std::vector<MagneticSymmetry> symmetries;
+		MagneticSpacegroup spacegroup;
+		// These are dependent on the input, should be moved to constructor?
+		std::array<double, 9> transformation_matrix;
+		std::array<double, 3> origin_shift;
+		// Smart reference to the input used
+		std::shared_ptr<Lattice> lattice;
+	};


I think for these we only need to store symmetries, spacegroup and lattice, what do you think?

SpglibDataset provides multiple different kinds of information.

Matrix representations of space group operations for input crystal structure

Space group type including setting, choice, point group, crystal class, that are covered by SpglibSpacegroupType and its associated information of the input crystal structure, Wyckoff positions, etc.

Crystal structure that standardizes the input crystal structure

Transformation matrix (P, p) from the input structure to the conventional unit cell

Primitive cell

Depending on purposes, most of these information are essential. All these were squashed in to the SpglibDataset just for convenience. Primitive cell information may not be necessary, although it was necessary at some stage in history.

Indeed and most of them will be made available indirectly via getters. The dataset itself should only store the minimum distinguishable information.

E.g. transformation matrix can be derived from lattice and std_lattice/primitive_lattice/spacegrouptype

MagbeticDataset I am definitely missing stuff there.

So far it seems I should include std_lattice and primitive_lattice

As for interface, I should add:

transformation matrices

wyckoff positions

equivalent atoms

Other things should be taken indirectly, e.g. hall_number through spacegroup (should be spacegroup_type here?)

I think the transformation matrix is the most important information.

In crystallography, we usually don't think about Cartesian coordinates. However, std_lattice contains (1) change-of-basis (transformation) matrix, (2) rigid rotation in Cartesian coordinates, (3) removal of distortion. So in addition, std_rotation_matrix is necessary to relate them. primitive_lattice is an intermediate information and it is not standardized. Probably this information is rarely used. To make primitive cell, we can transform the input crystal structure using the transformation matrix, then we can apply a pre-defined transformation matrix to the primitive cell, https://spglib.readthedocs.io/en/stable/definition.html#transformation-to-the-primitive-cell. This is a cleaner approach.

The strong point of spglib is the feature of removing distortion. The way of the treatment of distortion is not unique, and is somehow implemented algorithmically not mathematically in spglib. These naive information are averaged out and pushed into each element of the dataset. Spglib pretends working properly, but no one knows its super details.

Signed-off-by: Cristian Le <[email protected]> Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the cpp branch 2 times, most recently from c402ca8 to 3dc1dd1 Compare June 18, 2023 12:54

LecrisUT self-assigned this Jun 18, 2023

LecrisUT added the enhancement Significant ehancements label Jun 18, 2023

LecrisUT force-pushed the cpp branch 3 times, most recently from 7cefe14 to 958eb41 Compare June 20, 2023 12:50

LecrisUT mentioned this pull request Sep 30, 2023

Test fixture #303

Draft

22 tasks

LecrisUT added this to the 2.4 milestone Jan 25, 2024

LecrisUT force-pushed the cpp branch 2 times, most recently from a7895de to 22ffd64 Compare January 26, 2024 17:36

LecrisUT modified the milestones: 2.4, 2.5 Feb 15, 2024

LecrisUT force-pushed the cpp branch from 22ffd64 to 743385b Compare February 15, 2024 14:25

LecrisUT force-pushed the cpp branch from 743385b to af390e4 Compare February 28, 2024 18:39

LecrisUT commented Feb 28, 2024

View reviewed changes

[WIP] Add basic C++ interface

c84181b

Signed-off-by: Cristian Le <[email protected]> Signed-off-by: Cristian Le <[email protected]>

LecrisUT force-pushed the cpp branch from af390e4 to c84181b Compare May 9, 2024 13:29

LecrisUT mentioned this pull request May 9, 2024

Simplify datastructure #489

Open

LecrisUT modified the milestones: 2.5, 2.6 Jul 2, 2024

LecrisUT modified the milestones: 2.6, 2 8 Jun 16, 2025

Conversation

LecrisUT commented Jun 18, 2023

Uh oh!

codecov-commenter commented Jun 18, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LecrisUT Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jun 18, 2023 •

edited by codecov bot

Loading

LecrisUT Feb 29, 2024 •

edited

Loading