Skip to content

Comments

BREAKING CHANGE: Replace symmetry type with enum#586

Merged
IvanaGyro merged 5 commits intomasterfrom
refactor-symmetry
Aug 12, 2025
Merged

BREAKING CHANGE: Replace symmetry type with enum#586
IvanaGyro merged 5 commits intomasterfrom
refactor-symmetry

Conversation

@IvanaGyro
Copy link
Collaborator

@IvanaGyro IvanaGyro commented Mar 19, 2025

Changes

  • Replaced class SymmetryType_class with enum SymmetryType
  • Removed struct __sym (I mention here because C++ doesn't have name mangling like Python. Removing this structure indeed breaks the interface.)
  • Removed singleton instance SymType

Following Steps

I am going to remove the dependency of boost in the Symmetry class. There are two options:

  1. Merged class U1Symmetry and class ZnSymmetry into class Symmetry and used switch-like syntax to delegate the member functions
  2. Used std::variant to directly hold the instance of class U1Symmetry and class ZnSymmetry in Symmetry.

- Replaced `class SymmetryType_class` with `enum SymmetryType`
- Removed `struct __sym` (I mention here becuase C++ doesn't have name
mangling like Python. Removing this structure indeed breaks the
interface.)
- Removed singaleton instance `SymType`
@codecov
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

❌ Patch coverage is 33.33333% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 22.17%. Comparing base (31da063) to head (dec3770).
⚠️ Report is 42 commits behind head on master.

Files with missing lines Patch % Lines
include/Symmetry.hpp 37.50% 7 Missing and 3 partials ⚠️
src/Symmetry.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #586   +/-   ##
=======================================
  Coverage   22.17%   22.17%           
=======================================
  Files         211      211           
  Lines       51053    51044    -9     
  Branches    19010    19006    -4     
=======================================
  Hits        11319    11319           
+ Misses      37969    37960    -9     
  Partials     1765     1765           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hunghaoti
Copy link
Collaborator

I believe the original design intention was to allow for easy switching between Python and C++. For instance, if the Python code uses SymType.U, the C++ code would also use SymType.U.

If we change SymType to an enum , the syntax in C++ would become SymType::U. In my point of view it is acceptable. Since we have similar issue when using namespace in C++. For example, in Python we write cytnx.random.uniform(...), and in C++ we write cytnx::random::uniform(...). @kaihsin , what do you think about it?

If we decide to use enum, we can consider using enum class.

@ianmccul
Copy link
Contributor

eventually you will want to make this something more general, so that you can add new symmetries later, ideally as an extension that doesn't require modifying existing code.

@kaihsin
Copy link
Member

kaihsin commented Apr 11, 2025

I agree with @ianmccul . For now, yes. Let's do enum

py::enum_<__sym::__stype>(m, "SymType")
.value("Z", __sym::__stype::Z)
.value("U", __sym::__stype::U)
py::enum_<SymmetryType>(m, "SymType")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use "SymmetryType" here rather than "SymType".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will break the Python API. I can make SymType deprecated now, but it will not be removed until version 2.x.

@hunghaoti
Copy link
Collaborator

I suggest that in the future we can consider replacing enum with enum class.

@yingjerkao
Copy link
Collaborator

Is this PR good to merge?

@hunghaoti
Copy link
Collaborator

I agree this PR is good to merge. @IvanaGyro , what do you think?

@IvanaGyro
Copy link
Collaborator Author

I thought to merge this PR after changing SymType to SymmetryType and mark SymType deprecated. This job was pending because I set updating build process and supporting Mac at higher priority.

We can merge this PR now and add the new Python API in the future.

@IvanaGyro IvanaGyro merged commit 93d91f0 into master Aug 12, 2025
5 of 6 checks passed
@IvanaGyro IvanaGyro deleted the refactor-symmetry branch August 12, 2025 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants