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.

Sign up for GitHub

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

Jump to bottom

Convert result of multivariate polynomial evaluation into correct parent #35045

Merged
merged 5 commits into from
Mar 13, 2023
Merged

Convert result of multivariate polynomial evaluation into correct parent #35045

merged 5 commits into from
Mar 13, 2023

Conversation

mezzarobba
Copy link
Member

@mezzarobba mezzarobba commented Feb 9, 2023 •
edited

Importing trac/u/roed/incorrect_parent_when_evaluating_constant_multivariate_polynomial...

(meant to fix #33373)

@mezzarobba mezzarobba force-pushed the incorrect_parent_when_evaluating_constant_multivariate_polynomial branch from 6489cab to 77d5f5d Compare February 9, 2023 18:29
@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2023 •
edited

Codecov Report

Base: 88.59% // Head: 88.58% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (042fdc5) compared to base (104dde9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #35045      +/-   ##
===========================================
- Coverage    88.59%   88.58%   -0.01%     
===========================================
  Files         2136     2140       +4     
  Lines       396142   396962     +820     
===========================================
+ Hits        350948   351640     +692     
- Misses       45194    45322     +128
Impacted Files Coverage Δ
...dular/modform_hecketriangle/graded_ring_element.py 94.58% <100.00%> (ø)
...c/sage/schemes/riemann_surfaces/riemann_surface.py 88.52% <100.00%> (+<0.01%) ⬆️
src/sage/schemes/toric/points.py 96.53% <100.00%> (ø)
src/sage/rings/polynomial/pbori/parallel.py 59.82% <0.00%> (-1.79%) ⬇️
src/sage/graphs/generators/random.py 91.09% <0.00%> (-1.03%) ⬇️
src/sage/homology/matrix_utils.py 87.15% <0.00%> (-0.92%) ⬇️
src/sage/graphs/generic_graph.py 89.12% <0.00%> (-0.42%) ⬇️
.../sage/schemes/elliptic_curves/ell_curve_isogeny.py 94.92% <0.00%> (-0.40%) ⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py 94.09% <0.00%> (-0.40%) ⬇️
src/sage/combinat/tableau.py 95.86% <0.00%> (-0.35%) ⬇️
... and 61 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mezzarobba mezzarobba marked this pull request as ready for review February 10, 2023 13:41
@mezzarobba
Copy link
Member Author

I already reviewed @roed314's commit (on trac, see the linked issue), so we only need someone to take a look at mine.

@mkoeppe mkoeppe mentioned this pull request Feb 13, 2023
Closed
tscrim
tscrim requested changes Feb 17, 2023
View reviewed changes
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.

Just a few more tweaks, but LGTM otherwise.

src/sage/modular/modform_hecketriangle/graded_ring_element.py Outdated Show resolved Hide resolved
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx Outdated Show resolved Hide resolved
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx Outdated Show resolved Hide resolved
src/sage/schemes/riemann_surfaces/riemann_surface.py Outdated Show resolved Hide resolved
src/sage/schemes/toric/points.py
@@ -849,7 +849,7 @@ def inhomogeneous_equations(self, ring, nonzero_coordinates, cokernel):
z = [ring.zero()] * nrays
for i, value in zip(nonzero_coordinates, z_nonzero):
z[i] = value
return [poly(z) for poly in self.polynomials]
return [poly.change_ring(ring)(z) for poly in self.polynomials]

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

uh, the comprehension is over poly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, duh. I was thinking the loop was over z.

@mezzarobba mezzarobba force-pushed the incorrect_parent_when_evaluating_constant_multivariate_polynomial branch from 5f11491 to 042fdc5 Compare February 19, 2023 19:01
@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 042fdc5

tscrim
tscrim approved these changes Feb 20, 2023
View reviewed changes
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.

Thanks. LGTM. I think the failure in ell_field.py is unrelated to this ticket.

@mezzarobba
Copy link
Member Author

mezzarobba commented Feb 20, 2023 via email

@vbraun vbraun merged commit 2b03a1a into sagemath:develop Mar 13, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 13, 2023
@jhpalmieri jhpalmieri mentioned this pull request Mar 16, 2023
Closed
2 tasks
@jhpalmieri
Copy link
Member

Any ideas what's going on in #35273? That looks like it's connected to this: on OS X, all tests pass with 10.0.beta3, but with 10.0.beta3 + this ticket, there are failures.

@mezzarobba mezzarobba deleted the incorrect_parent_when_evaluating_constant_multivariate_polynomial branch December 6, 2023 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscrim tscrim tscrim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
sage-10.0
Development

Successfully merging this pull request may close these issues.

Incorrect parent when evaluating constant multivariate polynomial
7 participants
@mezzarobba @codecov-commenter @jhpalmieri @tscrim @vbraun @mkoeppe @roed314