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: |
Description (last modified by )
Change History (18)
comment:1 Changed 6 years ago by
Branch: | → u/tscrim/ticket/var_names_not_symbolic_names-23337 |
---|---|
Commit: | → e938684edebd1693f84db659f3bdeb05e4bfb5f8 |
Dependencies: | → #23337 |
Status: | new → needs_review |
comment:2 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 6 years ago by
Status: | needs_review → needs_work |
---|
comment:4 Changed 6 years ago by
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
Status: | needs_work → needs_review |
---|
comment:7 Changed 6 years ago by
Branch: | u/tscrim/ticket/var_names_not_symbolic_names-23337 → u/jdemeyer/ticket/var_names_not_symbolic_names-23337 |
---|
comment:8 Changed 6 years ago by
Authors: | Travis Scrimshaw → Travis Scrimshaw, Jeroen Demeyer |
---|---|
Commit: | e938684edebd1693f84db659f3bdeb05e4bfb5f8 → 7f4e92f1fa7d49263313fc1dd666a64e34b9e8c1 |
Description: | modified (diff) |
Reviewers: | → Jeroen Demeyer |
Summary: | Handling flatten.py and some mild cleanup → Clean up SpecializationMorphism |
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
|
comment:9 follow-up: 10 Changed 6 years ago by
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 Changed 6 years ago by
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
Commit: | 7f4e92f1fa7d49263313fc1dd666a64e34b9e8c1 → a28bb4e10aa59dd5291a54ce32e0707525d5ab96 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
a28bb4e | Minor fixes
|
comment:15 Changed 6 years ago by
Commit: | a28bb4e10aa59dd5291a54ce32e0707525d5ab96 → 6766161d710d43fdcc0ec73c0a2f08196fc68789 |
---|---|
Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
6766161 | Add UTF-8 encoding header
|
comment:16 Changed 6 years ago by
Status: | needs_review → positive_review |
---|
comment:17 Changed 6 years ago by
Reviewers: | Jeroen Demeyer → Jeroen Demeyer, Travis Scrimshaw |
---|
comment:18 Changed 6 years ago by
Branch: | u/jdemeyer/ticket/var_names_not_symbolic_names-23337 → 6766161d710d43fdcc0ec73c0a2f08196fc68789 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
Use variable names instead of symbolic variables
Handling flatten.py and some mild cleanup.