Add `get_magnetic_spacegroup_type_from_symmetry` and introduce dataclass output in python interface #485

lan496 · 2024-05-06T01:14:57Z

Closes #482

codecov · 2024-05-06T01:20:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.90%. Comparing base (9c8ec60) to head (f7e8b2f).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #485      +/-   ##
===========================================
+ Coverage    83.87%   83.90%   +0.03%     
===========================================
  Files           25       25              
  Lines         8167     8184      +17     
  Branches      1702     1709       +7     
===========================================
+ Hits          6850     6867      +17     
  Misses        1317     1317

Flag	Coverage Δ
c_api	`74.78% <ø> (ø)`
fortran_api	`56.19% <ø> (ø)`
python_api	`80.35% <100.00%> (+0.35%)`	⬆️
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 · 2024-05-06T05:13:58Z

Can you also add spacegrouptype while you're at it?

LecrisUT · 2024-05-06T05:22:38Z

python/spglib/spglib.py

+    msg_type_list = _spglib.magnetic_spacegroup_type_from_symmetry(
+        rots, trans, timerev, latt, symprec
+    )
+    _set_error_message()


We have started to make functions raise error, maybe we should make this function raise errors as well. For compatibility it should be gated by a kwarg and we should start encouraging users to set it and use pythonic try catch logics

LecrisUT · 2024-05-06T05:35:05Z

python/_spglib.c

+    double(*lattice)[3];
+    int num_operations;
+
+    SpglibMagneticSpacegroupType msg_type;


Can we start moving the type definitions next to where they are used? Can help with some optimizations and some readability by decluttering the variables and scoping the lifetimes

Yes. Since we are free from some older compilers, my understanding is that we can start moving the location of type definitions

python/spglib/spglib.py

LecrisUT · 2024-05-06T07:40:50Z

python/_spglib.c

+    lattice = (double(*)[3])PyArray_DATA(py_lattice);
+    num_operations = PyArray_DIMS(py_rotations)[0];
+
+    msg_type = spg_get_magnetic_spacegroup_type_from_symmetry(


There are a few interfaces that do not properly set error when they fail. Can you check them?

To clarify on the C side of the code in spglib.c. At least anything that is public on spglib.h should have some error setting.

LecrisUT

One more round of comments. Before the release we should migrate the other dict as well.

ChangeLog.md

python/spglib/spglib.py

ChangeLog.md

LecrisUT

Also don't forget to rebase. CI is not running right now.

ChangeLog.md

python/spglib/spglib.py

Co-authored-by: Cristian Le <github@lecris.me>

python/spglib/spglib.py

Co-authored-by: Cristian Le <github@lecris.me>

LecrisUT

LGTM. The commit history is a bit noisy, so use squash commit if you don't plan on interactively rebasing.

LecrisUT · 2024-05-11T14:39:41Z

python/spglib/spglib.py

@@ -97,6 +111,88 @@ class SpglibError:
 spglib_error = SpglibError()


+@dataclasses.dataclass(eq=True, frozen=True)
+class DictInterface(Mapping[str, "Any"]):


Did you find that@dataclass was needed for this?

Yes, mypy seems to need it.

lan496 added this to the 2.5 milestone May 6, 2024

lan496 mentioned this pull request May 6, 2024

Release v2.5.0 #486

Closed

4 tasks

lan496 marked this pull request as ready for review May 6, 2024 02:17

lan496 requested a review from LecrisUT May 6, 2024 02:17

LecrisUT reviewed May 6, 2024

View reviewed changes

lan496 requested a review from LecrisUT May 6, 2024 10:54

LecrisUT requested changes May 8, 2024

View reviewed changes

LecrisUT requested changes May 9, 2024

View reviewed changes

lan496 requested a review from LecrisUT May 9, 2024 22:32

LecrisUT requested changes May 10, 2024

View reviewed changes

lan496 and others added 12 commits May 10, 2024 21:43

Add TypedDict for magnetic space group type

23e8045

get_magnetic_spacegroup_type_from_symmetry in python interface

e2b2516

Space-group type in TypedDict

0120700

Set error messages

ed380f7

Introduce dataclass for returned objects

e8b572d

address review comments

93842b1

Update ChangeLog.md

ad7b6e8

Co-authored-by: Cristian Le <github@lecris.me>

Update ChangeLog.md

22e69c6

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

a4172be

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

5a99943

Co-authored-by: Cristian Le <github@lecris.me>

Fix type annotation for input arrays

1496a57

fix

9c28457

lan496 force-pushed the get_magnetic_spacegroup_type_from_symmetry branch from 6c2e2e6 to 9c28457 Compare May 10, 2024 12:46

LecrisUT requested changes May 10, 2024

View reviewed changes

python/spglib/spglib.py Outdated Show resolved Hide resolved

python/spglib/spglib.py Outdated Show resolved Hide resolved

lan496 and others added 3 commits May 11, 2024 09:02

Update ChangeLog.md

f6a7448

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

13ba6c5

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

e001862

Co-authored-by: Cristian Le <github@lecris.me>

lan496 and others added 7 commits May 11, 2024 09:03

Update python/spglib/spglib.py

36d3cdc

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

6b64f7d

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

84bb031

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

e00eeb4

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

342005c

Co-authored-by: Cristian Le <github@lecris.me>

Update python/spglib/spglib.py

0adcf94

Co-authored-by: Cristian Le <github@lecris.me>

lint and fix

f7e8b2f

lan496 requested a review from LecrisUT May 11, 2024 01:05

LecrisUT approved these changes May 11, 2024

View reviewed changes

lan496 merged commit 0030689 into spglib:develop May 11, 2024

lan496 deleted the get_magnetic_spacegroup_type_from_symmetry branch May 11, 2024 23:55

lan496 changed the title ~~Add get_magnetic_spacegroup_type_from_symmetry in python interface~~ Add get_magnetic_spacegroup_type_from_symmetry and introduce dataclass output in python interface May 12, 2024

lan496 mentioned this pull request May 12, 2024

dataclass output for Python API #491

Closed

LecrisUT linked an issue Jul 2, 2024 that may be closed by this pull request

dataclass output for Python API #491

Closed

Add get_magnetic_spacegroup_type_from_symmetry and introduce dataclass output in python interface #485

Add get_magnetic_spacegroup_type_from_symmetry and introduce dataclass output in python interface #485

Uh oh!

Conversation

lan496 commented May 6, 2024

Uh oh!

codecov bot commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LecrisUT commented May 6, 2024

Uh oh!

LecrisUT May 6, 2024

Choose a reason for hiding this comment

Uh oh!

LecrisUT May 6, 2024

Choose a reason for hiding this comment

Uh oh!

lan496 May 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LecrisUT May 6, 2024

Choose a reason for hiding this comment

Uh oh!

LecrisUT May 6, 2024

Choose a reason for hiding this comment

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

LecrisUT May 11, 2024

Choose a reason for hiding this comment

Uh oh!

lan496 May 11, 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!

Uh oh!

Add `get_magnetic_spacegroup_type_from_symmetry` and introduce dataclass output in python interface #485

Add `get_magnetic_spacegroup_type_from_symmetry` and introduce dataclass output in python interface #485

codecov bot commented May 6, 2024 •

edited

Loading

lan496 May 11, 2024 •

edited

Loading