BREAKING CHANGE: Replace symmetry type with enum
Changes
- Replaced
class SymmetryType_classwithenum 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:
- Merged
class U1Symmetryandclass ZnSymmetryintoclass Symmetryand used switch-like syntax to delegate the member functions - Used
std::variantto directly hold the instance ofclass U1Symmetryandclass ZnSymmetryinSymmetry.
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.
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.
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.
I agree with @ianmccul . For now, yes. Let's do enum
I suggest that in the future we can consider replacing enum with enum class.
Is this PR good to merge?
I agree this PR is good to merge. @IvanaGyro , what do you think?
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.