Skip to content

Add functionality for the fpsemi-examples presentations #85

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

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

MTWhyte
Copy link
Contributor

@MTWhyte MTWhyte commented Nov 26, 2022

Note: This PR is on top of #84

This PR adds functionality for the fpsemi-examples presentations, addressing issue #83. It is not entirely finished, but in a state where @james-d-mitchell and I might discuss finishing touches. This might include:

  • I haven't yet documented addition on the author values, since I'm not sure how this should look.
  • There is a bug, I think possibly related to setting default author values, which I'll describe in a comment below. I've kept the default author value for symmetric_group, but not set this up for other functions yet. I'll do this after we've figured out what's going on.
  • I will also double check everything's in order (mainly the documentation) before marking this as ready for review.

@MTWhyte MTWhyte marked this pull request as draft November 26, 2022 17:02
@james-d-mitchell
Copy link
Member

The CI will fail until we release a version of libsemigroups including the functionality that you're exposing here, so that's expected! If this didn't highlight any issues with libsemigroups itself, then I can make that release early next week.

@MTWhyte
Copy link
Contributor Author

MTWhyte commented Nov 26, 2022

The author value bug

Where author values are given as the sole argument to these functions, things sometimes break (as in, Python freezes for a while, and then gets killed). This is not the case for every function. Playing with it now, it appears to be the as @james-d-mitchell suspected, with author values being interpreted as integers. The following is evidence for this:

In [1]: from libsemigroups_pybind11.fpsemigroup import brauer_monoid, author

In [2]: author.Carmichael
Out[2]: <author.Carmichael: 4>

In [3]: author.Machine
Out[3]: <author.Machine: 0>

In [4]: brauer_monoid(author.Carmichael) == brauer_monoid(4)
Out[4]: True

In [5]: brauer_monoid(author.Machine)
[1]    79396 segmentation fault  ipython

(The segfault with being since in libsemigroups, fpsemigroup::brauer_monoid takes a size_t argument).

@MTWhyte
Copy link
Contributor Author

MTWhyte commented Nov 26, 2022

The best __repr__ for the authors would be to ditch the associated number, since that shouldn't really be relevant to the user. I tried to define this but wasn't successful, so that's also something for the todo list.

@james-d-mitchell
Copy link
Member

@MTWhyte should be possible to take this to completion now!

@MTWhyte MTWhyte marked this pull request as ready for review December 2, 2022 10:22
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.

A few minor changes, then I'm happy to merge, thanks @MTWhyte

@james-d-mitchell james-d-mitchell added the new-feature A label for PRs that contain new features label Dec 6, 2022
@james-d-mitchell james-d-mitchell merged commit d1209d3 into libsemigroups:main Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A label for PRs that contain new features
Projects
Development

Successfully merging this pull request may close these issues.

2 participants