-
Notifications
You must be signed in to change notification settings - Fork 10
Sync to<Presentation> upstream enhancements #358
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
Conversation
|
@james-d-mitchell not sure if this is 100% correct; I think I got all the in/out cases but may have missed something. I haven't updated docs or table yet but just wanted to give you a chance to take a look at the code. |
james-d-mitchell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jswent this looks pretty good, the only question I have is, if you do, e.g. to(k, rtype=(Presentation, str)) and k is a KnuthBendix object using strings, does to(k, rtype=(Presentation, str)) is k.presentation() return True or False?
Also you will likely have to add some code to the python part of the package and some tests.
For Did you mean for
Sounds good. |
|
Okay @james-d-mitchell so I think I may have found the culprit. I added two separate bindings for However it still calls the different word binding, which I originally thought was a bug in my code. Here's my demo example: Upon further investigation, it actually is operating correctly as when the Kambites object is initialized it doesn't use Which is caused by the way the libsemigroups_pybind11/src/libsemigroups_pybind11/kambites.py Lines 34 to 40 in 6139b05
Is this intentional? It seems that there is no way to create a Should I go ahead with the separate bindings for the rest of these algorithms or just leave the value returns? EDIT: To clarify, reference return is working with Word type: But the Python objects are still different, as expected: |
|
Or @Joseph-Edwards would the above be your area? |
1347a19 to
2e2ef80
Compare
|
@james-d-mitchell I think this should be mostly ready for your review. Kambites, Congruence, and ToddCoxeter will return a reference to the cxx object (Python objects are still different) when the word types are the same. Added tests and docs. |
This PR adds support for Congruence, Kambites, Stephen, and ToddCoxeter objects to the
tofunction for rtype=Presentation