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

Implement dual subdivision and weight vectors for tropical variety #38536

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

Conversation

verreld7
Copy link
Contributor

@verreld7 verreld7 commented Aug 20, 2024

This PR introduces some new methods in TropicalVariety related to dual subdivision and weight vectors.

Summary of changes:

  1. Add dual_subdivision in TropicalVariety: return the dual subdivision for any tropical variety as a Graph object

  2. Add _components_of_vertices in TropicalCurve: intermediate method the help calculate weight vectors

  3. Add weight_vectors in TropicalCurve: return the weight vectors of each vertex

  4. Add weight_vectors in TropicalSurface: return the weight vectors of each unique line of intersection

  5. Add a few methods related to dual subdivision of tropical curve such as:

    • is_smooth
    • is_simple
    • genus
    • contribution

CC: @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

#37962: issues related to this PR
#38291: continuation of this PR

Copy link

github-actions bot commented Aug 22, 2024

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

@tscrim tscrim self-requested a review August 27, 2024 05:41
@verreld7 verreld7 marked this pull request as ready for review September 25, 2024 12:34
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.

Sorry for taking so long to get to this. Here are my comments.

src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_polynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_polynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_variety.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.

I think this is all of them that you did not change the case. Please be sure to run the doctests before pushing it.

src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.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.

Thank you. A few other little changes. This should be the last round.

Comment on lines -14 to +17
sage: (x + R(3)*x) * (x^2 + x)
3*x^3 + 3*x^2
sage: (x^2 + R(1)*x + R(-1))^2
0*x^4 + 1*x^3 + 2*x^2 + 0*x + (-2)
sage: (R(3)*x + R(1)) * (x^2 + x)
3*x^3 + 3*x^2 + 1*x
sage: (x^2 + x + R(0))^2
0*x^4 + 0*x^3 + 0*x^2 + 0*x + 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, why are you changing these tests? If you want to have different tests, just add them instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You haven't answered why these tests are changed instead of adding additional ones.

Copy link
Contributor Author

@verreld7 verreld7 Nov 6, 2024

Choose a reason for hiding this comment

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

The test was changed to make it clearer while testing the same functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just have both. More examples/tests is not a bad thing. :)

src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
@verreld7 verreld7 requested a review from tscrim November 5, 2024 07:39
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.

Also, the bot is reporting that the doc is not building. Have you checked locally?

src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_variety.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
src/sage/rings/semirings/tropical_mpolynomial.py Outdated Show resolved Hide resolved
sage: R.<x,y> = PolynomialRing(T)
sage: p1 = R(3) + R(2)*x + R(2)*y + R(3)*x*y + x^2 + y^2
sage: p1.dual_subdivision()
Polyhedral complex with 4 maximal cells
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should have it also display the maximal cell polytopes in this complex (with their explicit data) and also below.

Comment on lines -14 to +17
sage: (x + R(3)*x) * (x^2 + x)
3*x^3 + 3*x^2
sage: (x^2 + R(1)*x + R(-1))^2
0*x^4 + 1*x^3 + 2*x^2 + 0*x + (-2)
sage: (R(3)*x + R(1)) * (x^2 + x)
3*x^3 + 3*x^2 + 1*x
sage: (x^2 + x + R(0))^2
0*x^4 + 0*x^3 + 0*x^2 + 0*x + 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

You haven't answered why these tests are changed instead of adding additional ones.

@verreld7
Copy link
Contributor Author

verreld7 commented Nov 6, 2024

The html doc builds properly locally, but the pdf does not. I am still figuring out where the error is.

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.

This might fix the pdf docbuild issue; that can be more strict with the type of object linked. Other than that, I don't see any latex or formatting issues.

corresponding to the exponents of the monomials of tropical
polynomial.

OUTPUT: :class:`~sage.geometry.polyhedron.constructor.Polyhedron`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
OUTPUT: :class:`~sage.geometry.polyhedron.constructor.Polyhedron`
OUTPUT: :func:`~sage.geometry.polyhedron.constructor.Polyhedron`

Comment on lines -14 to +17
sage: (x + R(3)*x) * (x^2 + x)
3*x^3 + 3*x^2
sage: (x^2 + R(1)*x + R(-1))^2
0*x^4 + 1*x^3 + 2*x^2 + 0*x + (-2)
sage: (R(3)*x + R(1)) * (x^2 + x)
3*x^3 + 3*x^2 + 1*x
sage: (x^2 + x + R(0))^2
0*x^4 + 0*x^3 + 0*x^2 + 0*x + 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just have both. More examples/tests is not a bad thing. :)

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