Cytnx icon indicating copy to clipboard operation
Cytnx copied to clipboard

BREAKING CHANGE: Replace symmetry type with enum

Open IvanaGyro opened this issue 9 months ago • 5 comments

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.

IvanaGyro avatar Mar 19 '25 07:03 IvanaGyro

Codecov Report

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

Files with missing lines Patch % Lines
include/Symmetry.hpp 37.50% 7 Missing and 3 partials :warning:
src/Symmetry.cpp 0.00% 2 Missing :warning:
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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Mar 19 '25 07:03 codecov[bot]

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.

hunghaoti avatar Apr 10 '25 16:04 hunghaoti

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.

ianmccul avatar Apr 10 '25 23:04 ianmccul

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

kaihsin avatar Apr 11 '25 00:04 kaihsin

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

hunghaoti avatar Apr 11 '25 02:04 hunghaoti

Is this PR good to merge?

yingjerkao avatar Aug 11 '25 13:08 yingjerkao

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

hunghaoti avatar Aug 12 '25 00:08 hunghaoti

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 avatar Aug 12 '25 06:08 IvanaGyro