Opened 23 months ago

Closed 22 months 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) Commit: 6766161d710d43fdcc0ec73c0a2f08196fc68789
Dependencies: #23337 Stopgaps:

Description (last modified by jdemeyer)


Change History (18)

comment:1 Changed 23 months ago by jdemeyer

  • Branch set to u/tscrim/ticket/var_names_not_symbolic_names-23337
  • Commit set to e938684edebd1693f84db659f3bdeb05e4bfb5f8
  • Dependencies set to #23337
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 23 months ago by jdemeyer

  • Status changed from needs_review to needs_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 23 months 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 23 months ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:6 Changed 23 months ago by jdemeyer

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

comment:7 Changed 23 months ago by jdemeyer

  • Branch changed from u/tscrim/ticket/var_names_not_symbolic_names-23337 to u/jdemeyer/ticket/var_names_not_symbolic_names-23337

comment:8 Changed 23 months ago by jdemeyer

  • Authors changed from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer
  • Commit changed from e938684edebd1693f84db659f3bdeb05e4bfb5f8 to 7f4e92f1fa7d49263313fc1dd666a64e34b9e8c1
  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer
  • Summary changed from Handling flatten.py and some mild cleanup to Clean 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 follow-up: Changed 23 months 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 23 months 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 23 months ago by git

  • Commit changed from 7f4e92f1fa7d49263313fc1dd666a64e34b9e8c1 to a28bb4e10aa59dd5291a54ce32e0707525d5ab96

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

​a28bb4eMinor fixes

comment:12 Changed 23 months ago by jdemeyer

Minor update.

comment:13 Changed 23 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:14 Changed 23 months ago by chapoton

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

comment:15 Changed 23 months ago by git

  • Commit changed from a28bb4e10aa59dd5291a54ce32e0707525d5ab96 to 6766161d710d43fdcc0ec73c0a2f08196fc68789
  • Status changed from positive_review to needs_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 23 months ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:17 Changed 23 months ago by jdemeyer

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw

comment:18 Changed 22 months ago by vbraun

  • Branch changed from u/jdemeyer/ticket/var_names_not_symbolic_names-23337 to 6766161d710d43fdcc0ec73c0a2f08196fc68789
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.