Clean up SpecializationMorphism

[jdemeyer]

Depends on #23337

CC: @tscrim

Component: algebra

Author: Travis Scrimshaw, Jeroen Demeyer

Branch/Commit: 6766161

Reviewer: Jeroen Demeyer, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/23343

[jdemeyer]

New commits:

ee4548f	Use variable names instead of symbolic variables
e938684	Handling flatten.py and some mild cleanup.

[jdemeyer]

Branch: u/tscrim/ticket/var_names_not_symbolic_names-23337

[jdemeyer]

Commit: e938684

[jdemeyer]

Dependencies: #23337

[jdemeyer]

comment:3

Since you are cleaning this up anyway, I would prefer if you could also move the changes from #10483 to flatten.py to this branch. For example, it would allow to test whether the changes here are compatible with #10483.

[tscrim]

comment:4

I don't think they are really compatible. Instead, I think these changes should replace those in #10483. The changes of #10483 rely on strings for keys rather than variables, which (in principle) would not work if a variable name is repeated (I haven't explicitly tested that this works in my code or if this is [suppose to be] a supported feature).

[jdemeyer]

comment:6

OK, that wasn't clear. Thanks for the clarification.

[jdemeyer]

Changed branch from u/tscrim/ticket/var_names_not_symbolic_names-23337 to u/jdemeyer/ticket/var_names_not_symbolic_names-23337

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

Changed author from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer

[jdemeyer]

comment:8

In order to understand the SpecializationMorphism, I ended up doing a lot more clean up. Please review my commit.

---

New commits:

7f4e92f

Further clean up of SpecializationMorphism

[jdemeyer]

Changed commit from e938684 to 7f4e92f

[tscrim]

comment:9

Inserting at the beginning of a list repeatedly is more expensive than inserting at the end and then reversing (it has to bump each entry back every time you insert). Granted, at this scale it may not be a big issue.

Somewhere between paranoia and it makes the code easier to parse:

-            force_multivariate = (len(old) == 1) and is_MPolynomialRing(R)
+            force_multivariate = ((len(old) == 1) and is_MPolynomialRing(R))

Is there a definitive reason to force the multivariate polynomial ring? I know you're not changing the current implementation, but I'm just wondering if that is something that is necessary since the univariates tend to be more powerful. If you don't know, then we can just leave it as is.

LGTM otherwise.

[jdemeyer]

comment:10

Replying to @tscrim:

Is there a definitive reason to force the multivariate polynomial ring?

I don't know, I just didn't want to change the existing behaviour.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

a28bb4e

Minor fixes

[sagetrac-git]

Changed commit from 7f4e92f to a28bb4e

[jdemeyer]

comment:12

Minor update.

[tscrim]

comment:13

Thanks. LGTM.

[fchapoton]

comment:14

missing utf8 declaration in src/sage/rings/polynomial/flatten.py

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

6766161

Add UTF-8 encoding header

[sagetrac-git]

Changed commit from a28bb4e to 6766161

[jdemeyer]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw

[vbraun]

Changed branch from u/jdemeyer/ticket/var_names_not_symbolic_names-23337 to 6766161