Opened 6 years ago

Closed 6 years ago

#23343 closed enhancement (fixed)

Clean up SpecializationMorphism

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: algebra Keywords:
Cc: tscrim Merged in:
Authors: Travis Scrimshaw, Jeroen Demeyer Reviewers: Jeroen Demeyer, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 6766161 (Commits, GitHub, GitLab) Commit: 6766161d710d43fdcc0ec73c0a2f08196fc68789
Dependencies: #23337 Stopgaps:

GitHub link to the corresponding issue

Change History (18)

comment:1 Changed 6 years ago by jdemeyer

Branch: u/tscrim/ticket/var_names_not_symbolic_names-23337
Commit: e938684edebd1693f84db659f3bdeb05e4bfb5f8
Dependencies: #23337
Status: newneeds_review

New commits:

ee4548fUse variable names instead of symbolic variables
e938684Handling flatten.py and some mild cleanup.

comment:2 Changed 6 years ago by jdemeyer

Description: modified (diff)

comment:3 Changed 6 years ago by jdemeyer

Status: needs_reviewneeds_work

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.

comment:4 Changed 6 years ago by tscrim

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).

comment:5 Changed 6 years ago by jdemeyer

Status: needs_workneeds_review

comment:6 Changed 6 years ago by jdemeyer

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

comment:7 Changed 6 years ago by jdemeyer

Branch: u/tscrim/ticket/var_names_not_symbolic_names-23337u/jdemeyer/ticket/var_names_not_symbolic_names-23337

comment:8 Changed 6 years ago by jdemeyer

Authors: Travis ScrimshawTravis Scrimshaw, Jeroen Demeyer
Commit: e938684edebd1693f84db659f3bdeb05e4bfb5f87f4e92f1fa7d49263313fc1dd666a64e34b9e8c1
Description: modified (diff)
Reviewers: Jeroen Demeyer
Summary: Handling flatten.py and some mild cleanupClean up SpecializationMorphism

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


New commits:

7f4e92fFurther clean up of SpecializationMorphism

comment:9 Changed 6 years ago by tscrim

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.

comment:10 in reply to:  9 Changed 6 years ago by jdemeyer

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.

comment:11 Changed 6 years ago by git

Commit: 7f4e92f1fa7d49263313fc1dd666a64e34b9e8c1a28bb4e10aa59dd5291a54ce32e0707525d5ab96

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

a28bb4eMinor fixes

comment:12 Changed 6 years ago by jdemeyer

Minor update.

comment:13 Changed 6 years ago by tscrim

Status: needs_reviewpositive_review

Thanks. LGTM.

comment:14 Changed 6 years ago by chapoton

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

comment:15 Changed 6 years ago by git

Commit: a28bb4e10aa59dd5291a54ce32e0707525d5ab966766161d710d43fdcc0ec73c0a2f08196fc68789
Status: positive_reviewneeds_review

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

6766161Add UTF-8 encoding header

comment:16 Changed 6 years ago by jdemeyer

Status: needs_reviewpositive_review

comment:17 Changed 6 years ago by jdemeyer

Reviewers: Jeroen DemeyerJeroen Demeyer, Travis Scrimshaw

comment:18 Changed 6 years ago by vbraun

Branch: u/jdemeyer/ticket/var_names_not_symbolic_names-233376766161d710d43fdcc0ec73c0a2f08196fc68789
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.