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.

Sign up for GitHub

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

Jump to bottom

libbraiding version bump #38887

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

libbraiding version bump #38887

wants to merge 7 commits into from

Conversation

miguelmarco
Copy link
Contributor

Fixes #38886 by updating libbraiding package and adding new methods to braids.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

tscrim
tscrim requested changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@tscrim tscrim 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 little details. Otherwise LGTM.

src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/libs/braiding.pyx Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
@miguelmarco
Copy link
Contributor Author

Thanks for the quick review!

There is still a problem with the install of the new version of libbraiding that I don't know how to solve: in the CI environments the new version is not installed, and then the tests fail.

I guess it has something to do with the environment-3*.yaml files, which are automatically generated by conda-lock.

Do you know what should be the right way to proceed?

@tscrim
Copy link
Collaborator

tscrim commented Oct 31, 2024

No idea. That’s far beyond my knowledge.

enriqueartal
enriqueartal reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@enriqueartal enriqueartal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it OK, and I agreed with @tscrim comments. I add a couple of comments about documentation. Basically it is fine for me.

src/sage/groups/braid.py

def trajectory(self):
r"""
Return the braid's trajectory.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a little more explanation would help.

src/sage/groups/braid.py

def cyclic_slidings(self):
r"""
Return the braid's cyclic slidings.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing

@enriqueartal
Copy link
Contributor

Thanks for the quick review!

There is still a problem with the install of the new version of libbraiding that I don't know how to solve: in the CI environments the new version is not installed, and then the tests fail.

I guess it has something to do with the environment-3*.yaml files, which are automatically generated by conda-lock.

Do you know what should be the right way to proceed?

Is it a more general problem? I updated my system to Fedora 41, and sirocco did not work after a compilation because of python's version. And it could not be reinstalled because it was already there.

@7c6f434c 7c6f434c mentioned this pull request Nov 2, 2024
Draft
@miguelmarco
Copy link
Contributor Author

I think I managed to force the CI systems to install the right version. Tests should pass now.

@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit 19ec8c0; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@enriqueartal
Copy link
Contributor

Since the test pass, it seems OK to a positive review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enriqueartal enriqueartal enriqueartal approved these changes

@soehms soehms Awaiting requested review from soehms

@tscrim tscrim Awaiting requested review from tscrim

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
c: group theory c: packages: standard s: positive review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to libbraiding v1.3
3 participants
@miguelmarco @tscrim @enriqueartal