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

Addition of new presentations to Chow rings of matroids #39359

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

Conversation

25shriya
Copy link
Contributor

@25shriya 25shriya commented Jan 21, 2025

This PR focuses on adding the atom-free and simplicial presentation of Chow rings of matroids. see relevant issue
The former is taken from Definition 4.1 of this paper and the latter from Definition 3.2.1 of this paper.

@tscrim

📝 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

@25shriya 25shriya changed the title Added new presentations to Chow rings of matroids Addition of new presentations to Chow rings of matroids Jan 21, 2025
Copy link

github-actions bot commented Jan 21, 2025

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


class ChowRingIdeal_nonaug(ChowRingIdeal):
class ChowRingIdeal_nonaug_fy(ChowRingIdeal):
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might consider using register_unpickle_override for backwards compatibility. (Technically speaking, the old class should be deprecated.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused about the issue here, primarily because the version in my branch defines the FY presentation as ChowRingIdeal_nonaug_fy (I made that change long ago)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I created a pickle with the old version, then tried to unpickle it with the new version, it wouldn’t know how to deal with it because the class wouldn’t exist anymore (since its name has changed). This is a backwards incompatible change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But isn't register_unpickle_override used only when both versions exist?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it redirects the unpickling.

Copy link
Contributor Author

@25shriya 25shriya Mar 12, 2025

Choose a reason for hiding this comment

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

So, would I need to import all of the following from sage.misc.persist import (unpickle_override, register_unpickle_override, SageUnpickler) and implement another method or something?
Sorry I'm quite new to dealing with pickling

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, just thr unpickle_override. The path is set by strings. There are some examples around the code base, but i don't remember where offhand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope the new commit fixes this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does build-doc-pdf fail when build-doc passes?
I've checked every corner of the documentation. Not sure what I'm missing

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope the new commit fixes this

I think so, but you can check it by creating a pickle on the develop branch (using dumps() and saving the output string), then loading it with your PR (via loads(saved_string)).

@tscrim
Copy link
Collaborator

tscrim commented Mar 14, 2025

The pdf build is more sensitive than the html. You can usually find the issue by looking through the html doc.

@tscrim
Copy link
Collaborator

tscrim commented Mar 14, 2025

Indeed, I found it. \superseteq should be \supseteq as I recall.

@25shriya
Copy link
Contributor Author

Seems like linting is failing due to some other code not authored by me in this PR

@tscrim
Copy link
Collaborator

tscrim commented Mar 14, 2025

One failure in the kahler_algebras.py file due to a basis order change (I think).

@25shriya
Copy link
Contributor Author

25shriya commented Mar 15, 2025

What's wrong with the doctests check? The doctests pass locally in all edited files for me

@tscrim
Copy link
Collaborator

tscrim commented Mar 15, 2025

Looks like somehow the ordering of the output is different. I am not sure why ar this point though.

@25shriya
Copy link
Contributor Author

Do the tests pass locally for you?

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2025

It doesn't really matter. They should pass on all machines and this indicating that there might be something in the ordering that is machine dependent.

@25shriya
Copy link
Contributor Author

Is it worth running a sorted() on groebner_basis(), normal_basis() and basis() methods?

@tscrim
Copy link
Collaborator

tscrim commented Mar 17, 2025

We could, but it's overkill to do that in the code. It's a bit annoying to do it in the output too. I can only guess that it comes from iterating over the lattice of flats. Still not sure yet what the best thing to do now is. I will think about it.

@tscrim
Copy link
Collaborator

tscrim commented Mar 18, 2025

Also I get a different order in the doctests too.

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.

2 participants