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

Additions to the bigraded Betti number methods #36166

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

OP5642
Copy link
Contributor

@OP5642 OP5642 commented Aug 31, 2023

This pull request expands the functionality of bigraded_betti_number() methods of a SimplicialComplex by adding an additional parameter verbose. The verbose option increases verbosity while computing the invariant by printing the subcomplexes along with their non-trivial homology groups.

This PR is a part of #35640 (GSoC 2023).

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

@OP5642
Copy link
Contributor Author

OP5642 commented Aug 31, 2023

@tscrim

@tscrim
Copy link
Collaborator

tscrim commented Sep 1, 2023

This should not depend on #35875. Adding the bigraded methods should be done on that PR alone. These PRs are independent AFAICS.

I agree with the verbose option, but I don't agree with the tablular, at least using print(). If anything, it should return an actual table(-like) object. Although I am not convinced that it isn't something the user should just do on their own.

@OP5642 OP5642 closed this Sep 9, 2023
@OP5642 OP5642 deleted the bbn_improvements branch September 9, 2023 21:55
@OP5642 OP5642 restored the bbn_improvements branch September 9, 2023 21:55
@tscrim tscrim reopened this Sep 10, 2023
@OP5642
Copy link
Contributor Author

OP5642 commented Sep 17, 2023

Thank you for the review, @tscrim. I also modified the verbose option for bigraded_betti_numbers() method by adding the index (in the form (-i, 2j)) as a prefix, as well as one more doctest which demonstrates the verbose output. How should I deal with lines 4945 and 5024, which seem way too long?

@OP5642 OP5642 marked this pull request as ready for review September 22, 2023 07:47
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.

LGTM.

@vbraun
Copy link
Member

vbraun commented Sep 22, 2023

Merge conflict

@tscrim
Copy link
Collaborator

tscrim commented Sep 26, 2023

@OP5642 Can you git rebase this over the most recent develop (and then force push it)?

@OP5642
Copy link
Contributor Author

OP5642 commented Sep 26, 2023

Thank you, I think I managed to do so just now.

@github-actions
Copy link

Documentation preview for this PR (built with commit 037b616; changes) is ready! 🎉

@vbraun vbraun merged commit ce855de into sagemath:develop Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants