Skip to content

Conversation

@jswent
Copy link
Contributor

@jswent jswent commented Nov 20, 2025

This PR adds support for Congruence, Kambites, Stephen, and ToddCoxeter objects to the to function for rtype=Presentation

@jswent
Copy link
Contributor Author

jswent commented Nov 20, 2025

@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.

@jswent jswent marked this pull request as draft November 21, 2025 11:00
Copy link
Member

@james-d-mitchell james-d-mitchell left a 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.

@james-d-mitchell james-d-mitchell added the libsemigroups-feature-not-yet-supported Label for issues and PRs related to features of libsemigroups not yet supported here label Nov 21, 2025
@jswent
Copy link
Contributor Author

jswent commented Nov 21, 2025

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?

For KnuthBendix it returns false because (I think) they are always going to be different objects (the to function invoked when I tested this creates a new presentation iterating through kb.active_rules()). However, I haven't changed this code.

Did you mean for Kambites? If so, the answer is also no which is a good point as the binding always returns by value. I could create two separate bindings for value and ref returns, but when I looked at this originally it appeared to me that the to.py code always returns a new Python wrapper - so wouldn't Python still see it as two different objects in memory even though the underlying C++ reference is the same? I'm doing some testing right now but I can't even get it to return the c++ reference atm.

Also you will likely have to add some code to the python part of the package and some tests.

Sounds good.

@jswent
Copy link
Contributor Author

jswent commented Nov 21, 2025

Okay @james-d-mitchell so I think I may have found the culprit. I added two separate bindings for Kambites to test: bind_kambites_to_pres_same_word and bind_kambites_to_pres_diff_word.

However it still calls the different word binding, which I originally thought was a bug in my code. Here's my demo example:

>>> p = Presentation("ab")
>>> presentation.add_rule(p, "ab", "ba")
>>> presentation.add_rule(p, "aa", "a")
>>> presentation.add_rule(p, "bb", "b")

>>> k = Kambites(congruence_kind.twosided, p)
>>> result = to(k, rtype=(Presentation,str))

>>> id(to_cxx(result)) == id(to_cxx(k.presentation()))
False

Upon further investigation, it actually is operating correctly as when the Kambites object is initialized it doesn't use KambitesString, rather KambitesMultiViewString:

>>> k = Kambites(congruence_kind.twosided, p)
>>> type(k._cxx_obj)
<class '_libsemigroups_pybind11.KambitesMultiViewString'>

Which is caused by the way the Kambites python object is constructed:

class Kambites(_CongruenceCommon):
__doc__ = _KambitesWord.__doc__
_py_template_params_to_cxx_type = {
(list[int],): _KambitesWord,
(str,): _KambitesMultiViewString,
}

Is this intentional? It seems that there is no way to create a KambitesString from Python.

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:

>>> p = Presentation([0, 1])
>>> presentation.add_rule(p, [0, 1], [1, 0])
>>> presentation.add_rule(p, [0, 0], [0])
>>> k = Kambites(congruence_kind.twosided, p)

>>> type(p._cxx_obj)
<class '_libsemigroups_pybind11.PresentationWord'>
>>> type(k._cxx_obj)
<class '_libsemigroups_pybind11.KambitesWord'>

>>> result = to(k, rtype=(Presentation, list[int]))
>>> id(result._cxx_obj) == id(k.presentation()._cxx_obj)
True

But the Python objects are still different, as expected:

>>> result is k.presentation()
False

@jswent
Copy link
Contributor Author

jswent commented Nov 21, 2025

Or @Joseph-Edwards would the above be your area?

@jswent
Copy link
Contributor Author

jswent commented Nov 21, 2025

@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.

@jswent jswent marked this pull request as ready for review November 21, 2025 16:33
@james-d-mitchell james-d-mitchell merged commit e5ecb4a into libsemigroups:main Nov 24, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libsemigroups-feature-not-yet-supported Label for issues and PRs related to features of libsemigroups not yet supported here

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants