Opened 2 years ago
Closed 2 years ago
#23337 closed enhancement (fixed)
Use variable names instead of symbolic variables
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  basic arithmetic  Keywords:  
Cc:  SimonKing, rws  Merged in:  
Authors:  Simon King, Ralf Stephan, Jeroen Demeyer  Reviewers:  Jeroen Demeyer, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  ee4548f (Commits)  Commit:  ee4548f920444ef5739ccc10cb67d9ce73449d85 
Dependencies:  Stopgaps: 
Description
Where possible, use QQ["x"]
instead of QQ[x]
to construct polynomial rings. After #10483, the latter will be deprecated.
Change History (15)
comment:1 Changed 2 years ago by
 Branch set to u/jdemeyer/ticket/23337
comment:2 Changed 2 years ago by
 Commit set to d1f6fff086983e2224af518ac55faeec1593ff63
comment:3 Changed 2 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Commit changed from d1f6fff086983e2224af518ac55faeec1593ff63 to 52de325f447e0779bed655d0e57727c903b0f33e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
52de325  Use variable names instead of symbolic variables

comment:5 Changed 2 years ago by
There are some doctest failures (see patchbot). There are some actual changes in output type, but I am not sure if they are (more) correct or not. I would think some of them are actual regressions since they do not contain as much information (i.e., ZZ[x][y]
has slightly different structure for things like factoring compared to ZZ[x,y]
).
comment:6 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:7 Changed 2 years ago by
 Commit changed from 52de325f447e0779bed655d0e57727c903b0f33e to ee4548f920444ef5739ccc10cb67d9ce73449d85
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ee4548f  Use variable names instead of symbolic variables

comment:8 Changed 2 years ago by
 Status changed from needs_work to needs_review
I reverted the changes to flatten.py
.
comment:9 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:10 Changed 2 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw
 Status changed from positive_review to needs_work
I think I understand what is going wrong with flatten.py
. It is checking the keys in D
, which are actual variables and hence, do not compare with the string values. So the variable names all get included when constructing new_vars
on this line:
new_vars = [[[t for t in v if t not in newD.keys()], b] for v,b in old_vars]
I think I see a good way to fix this. I also am going to do a slight bit of cleanup of that method.
comment:11 Changed 2 years ago by
Given that I reverted the changes to flatten.py
and that the previouslyfailing doctests now pass, why was this set to needs_work?
comment:12 Changed 2 years ago by
 Branch changed from u/jdemeyer/ticket/23337 to u/tscrim/ticket/var_names_not_symbolic_names23337
 Commit changed from ee4548f920444ef5739ccc10cb67d9ce73449d85 to e938684edebd1693f84db659f3bdeb05e4bfb5f8
 Status changed from needs_work to needs_review
This fixes the failing doctests but addresses the problem in flatten.py
(with some simple cleanup).
New commits:
e938684  Handling flatten.py and some mild cleanup.

comment:13 Changed 2 years ago by
I would rather fix the problem than push it off until later.
comment:14 Changed 2 years ago by
 Branch changed from u/tscrim/ticket/var_names_not_symbolic_names23337 to u/jdemeyer/ticket/23337
 Commit changed from e938684edebd1693f84db659f3bdeb05e4bfb5f8 to ee4548f920444ef5739ccc10cb67d9ce73449d85
 Status changed from needs_review to positive_review
comment:15 Changed 2 years ago by
 Branch changed from u/jdemeyer/ticket/23337 to ee4548f920444ef5739ccc10cb67d9ce73449d85
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Use variable names instead of symbolic variables