Skip to content

Conversation

@mantepse
Copy link
Contributor

@mantepse mantepse commented Feb 12, 2025

This fixes a performance bottleneck in #38108, which boiled down to the fact that producing the repr of generators in multivariate polynomial rings is expensive.

@saraedum
Copy link
Member

saraedum commented Feb 12, 2025

Looks good to me. If I forget to do it, feel free to set to positive review when the relevant tests pass.

@mantepse mantepse added the sd128 tickets of Sage Days 128 Le Teich label Feb 12, 2025
@github-actions
Copy link

github-actions bot commented Feb 12, 2025

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

@saraedum
Copy link
Member

 File "src/sage/rings/semirings/tropical_mpolynomial.py", line 551, in sage.rings.semirings.tropical_mpolynomial.TropicalMPolynomial.dual_subdivision
Warning: slow doctest:
    pc = p3.dual_subdivision(); pc
Test ran for 22.50s cpu, 22.14s wall
Check ran for 0.00s cpu, 0.00s wall
**********************************************************************
File "src/sage/rings/semirings/tropical_mpolynomial.py", line 584, in sage.rings.semirings.tropical_mpolynomial.TropicalMPolynomial.dual_subdivision
Warning: slow doctest:
    pc = p1.dual_subdivision(); pc
Test ran for 6.07s cpu, 6.01s wall
Check ran for 0.00s cpu, 0.00s wall
**********************************************************************
File "src/sage/rings/semirings/tropical_mpolynomial.py", line 904, in sage.rings.semirings.tropical_mpolynomial.TropicalMPolynomialSemiring.gen
Failed example:
    R.gen()
Expected:
    0*a
Got:
    a
**********************************************************************
File "src/sage/rings/semirings/tropical_mpolynomial.py", line 906, in sage.rings.semirings.tropical_mpolynomial.TropicalMPolynomialSemiring.gen
Failed example:
    R.gen(2)
Expected:
    0*c
Got:
    c
**********************************************************************
File "src/sage/rings/semirings/tropical_mpolynomial.py", line 927, in sage.rings.semirings.tropical_mpolynomial.TropicalMPolynomialSemiring.tuple
Failed example:
    R.gens()
Expected:
    (0*x0, 0*x1, 0*x2, 0*x3, 0*x4)
Got:
    (x0, x1, x2, x3, x4)

@saraedum
Copy link
Member

I don't know what these semirings do, but I guess they inherit from us and want some weird printing to be happy.

@saraedum
Copy link
Member

Some people who know about these things here at sd128 say that x is better than 0*x as an output here. So we should just update the doctests.

@mantepse
Copy link
Contributor Author

I simply adapted the repr of TropicalMPolynomial, because otherwise the output would have been inconsistent.

@mantepse
Copy link
Contributor Author

I cannot reproduce the failed test in linear_code.py.

The failure produced by

sage -t --warn-long 5.0 --random-seed=303265940927389845190418669274383562872 src/sage/schemes/elliptic_curves/ell_point.py             

is real and also present in develop, I think it is #39191

@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2025

When making the tropical polynomials, it was a deliberate design to make it 0*x to emphasize the tropicalness and to make it "copy-pasteable" since 1*x carries a different meaning. If you change 0*x -> x, then please make sure 1*x is not also printed as x (with a doctest).

@mantepse
Copy link
Contributor Author

Ah, that makes sense! The current PR does not change that!

@saraedum saraedum closed this Feb 13, 2025
@saraedum saraedum deleted the multi_polynomial_element/repr branch February 13, 2025 20:11
@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2025

What is the rationale for closing this (and the other PR) and reopening a new PR?

@mantepse
Copy link
Contributor Author

It is about the irrational coincidence that Martin had super powers and played on the wrong playground, and Martin does not know how to change the branch of a pull request.

I am really sorry and even more embarassed.

@tscrim
Copy link
Collaborator

tscrim commented Feb 14, 2025

I see. No problem. Well, there might have been another way to fix this, but this works. We can chat after your SageDays are over about alternatives. In the meantime, please enjoy some coffee and SageDays. I hope they are going well.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39523: multi polynomial element/repr
    
This is an identical replacement for sagemath#39502 which had to be closed.
    
URL: sagemath#39523
Reported by: Martin Rubey
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39523: multi polynomial element/repr
    
This is an identical replacement for sagemath#39502 which had to be closed.
    
URL: sagemath#39523
Reported by: Martin Rubey
Reviewer(s): Travis Scrimshaw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: commutative algebra sd128 tickets of Sage Days 128 Le Teich t: performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants