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 Chow ring ideal and Chow ring classes #38281

Merged
merged 178 commits into from
Nov 16, 2024

Conversation

25shriya
Copy link
Contributor

@25shriya 25shriya commented Jun 26, 2024

This PR is focused on addition of classes for Chow ring ideal and Chow ring of matroids.
Check relevant issue.
The ideals classes consist of the Chow ring ideal and Augmented Chow ring ideal, with Gröbner basis for each of them.

The Chow ring class is an initial version.
@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

Copy link

github-actions bot commented Jun 26, 2024

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

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.

Some little details, but the biggest thing in the multiplication table doctest is now failing. Likely this is due to different gens().

src/sage/matroids/matroid.pyx Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
src/sage/matroids/chow_ring_ideal.py Outdated Show resolved Hide resolved
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. If we find any issues later on, we can fix them at that point.

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 7, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

This PR is focused on addition of classes for Chow ring ideal and Chow
ring of matroids.
[Check relevant issue.](sagemath#37987)
The ideals classes consist of the Chow ring ideal and Augmented Chow
ring ideal, with Gröbner basis for each of them.

The Chow ring class is an initial version.
@tscrim


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38281
Reported by: 25shriya
Reviewer(s): 25shriya, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 8, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

This PR is focused on addition of classes for Chow ring ideal and Chow
ring of matroids.
[Check relevant issue.](sagemath#37987)
The ideals classes consist of the Chow ring ideal and Augmented Chow
ring ideal, with Gröbner basis for each of them.

The Chow ring class is an initial version.
@tscrim


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38281
Reported by: 25shriya
Reviewer(s): 25shriya, Travis Scrimshaw
@vbraun
Copy link
Member

vbraun commented Nov 9, 2024

On 32-bit Linux:

**********************************************************************
File "src/sage/matroids/chow_ring_ideal.py", line 660, in sage.matroids.chow_ring_ideal.AugmentedChowRingIdeal_atom_free._gens_constructor
Failed example:
    ch.defining_ideal()._gens_constructor(ch.defining_ideal().ring())
Expected:
    [A0*A1, A0*A2, A1*A2, A0^2 + 2*A0*A3 + A3^2, A1^2 + 2*A1*A3 + A3^2,
     A0*A1 + A0*A3, A2^2 + 2*A2*A3 + A3^2, A0*A2 + A0*A3,
     A0*A1 + A1*A3, A1*A2 + A1*A3, A0*A2 + A2*A3, A1*A2 + A2*A3]
Got:
    [A0*A1,
     A0*A2,
     A1*A2,
     A2^2 + 2*A2*A3 + A3^2,
     A0*A2 + A0*A3,
     A1^2 + 2*A1*A3 + A3^2,
     A0*A1 + A0*A3,
     A0^2 + 2*A0*A3 + A3^2,
     A1*A2 + A1*A3,
     A0*A1 + A1*A3,
     A1*A2 + A2*A3,
     A0*A2 + A2*A3]
**********************************************************************

@tscrim
Copy link
Collaborator

tscrim commented Nov 9, 2024

Curious. It seems one of the iterator is machine dependent.

@25shriya Just run a sorted on the output (probably best to do this for all outputs of _gens_constructor…)

@tscrim
Copy link
Collaborator

tscrim commented Nov 11, 2024

That should fix the issue.

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 13, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

This PR is focused on addition of classes for Chow ring ideal and Chow
ring of matroids.
[Check relevant issue.](sagemath#37987)
The ideals classes consist of the Chow ring ideal and Augmented Chow
ring ideal, with Gröbner basis for each of them.

The Chow ring class is an initial version.
@tscrim


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38281
Reported by: 25shriya
Reviewer(s): 25shriya, Travis Scrimshaw
@vbraun vbraun merged commit f63dd37 into sagemath:develop Nov 16, 2024
18 of 21 checks passed
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