Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rewrite the definitions of groups in symmetry.jl #32

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

kalmarek
Copy link
Contributor

you can check the correctness of group definitions using

include(joinpath(pathof(GroupsCore), "..", "..", "test", "conformance_test.jl"))
let G = KleinGroup()
    test_Group_interface(G)
    test_GroupElement_interface(rand(G, 2)...)
    nothing
end
let G = KleinPermGroup()
    test_Group_interface(G)
    test_GroupElement_interface(rand(G, 2)...)
    nothing
end
let G = CyclicGroup(12)
    test_Group_interface(G)
    test_GroupElement_interface(rand(G, 2)...)
    nothing
end
let G = lattice1group(5)
    test_Group_interface(G)
    test_GroupElement_interface(rand(G, 2)...)
    nothing
end

@blegat
Copy link
Owner

blegat commented Sep 14, 2023

Wow, that was fast. Was it incorrect before ?

@kalmarek
Copy link
Contributor Author

yeah, I found them a bit earlier ;) took me ~2h to rewrite;

I'm not sure it was incorrect, it didn't pass the tests because of missing methods.

@kalmarek
Copy link
Contributor Author

following interfaces is nice ;) everything is green here, locally ;)

@blegat blegat merged commit 34c691d into blegat:bl/rename Sep 14, 2023
@blegat
Copy link
Owner

blegat commented Sep 14, 2023

So does that mean that SymbolicWedderburn was not running enough checks on the group ?

@blegat
Copy link
Owner

blegat commented Sep 14, 2023

Maybe https://discourse.julialang.org/t/symmetry-sos-optimization-compatibility-issues/98057/6 has a similar issue, the group is not valid but with more checks an error would be thrown

@kalmarek
Copy link
Contributor Author

SymbolicWedderburn doesn't do any checks per se (it does now for the group action);
Here I just made sure that our groups comply to the group interface. User should do that on his own, I suppose?

@blegat
Copy link
Owner

blegat commented Sep 14, 2023

I think it's best if we do it by default and then the user can disable it with options. The power users will set the option to disable but the new user that does not know the option, he will also most probably have errors in the group ^^

@kalmarek
Copy link
Contributor Author

ok, I'll think about it

blegat added a commit that referenced this pull request Sep 26, 2023
* Update to MP renamings

* Use MP#bl/rename

* Fixes

* Fixes

* Fix ci

* Remove Sequences

* Updates

* Fixes

* Fix doc script

* Checkout v3

* Fix

* Remove COI

* Update symmetric_group

* Fixes

* Fix

* Fixes

* rewrite the definitions of groups in symmetry.jl (#32)

* rewrite the definitions of groups in symmetry.jl

* add Random

* Update src/symmetry.jl

Co-authored-by: Marek Kaluba <[email protected]>

* Fixes

* fixes after the rewrite of symmetry.jl (#33)

* sort out deepcopy_internal stuff

* move imports/exports away from symmetry.jl

* update action.jl to the rewrite of symmetry.jl

* Fixes

* Use latest SOS

* Update GH action

---------

Co-authored-by: Marek Kaluba <[email protected]>
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.

2 participants