sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 482 forks source link

py3: fix FriCAS interface #27268

Closed mantepse closed 5 years ago

mantepse commented 5 years ago

In python3, dict.items cannot be indexed. The processing of rootOf expression assumed this, so the design pattern must be changed.

Depends on #25899

CC: @fchapoton

Component: python3

Keywords: FriCAS

Author: Martin Rubey

Branch/Commit: 9a6ec3d

Reviewer: Travis Scrimshaw

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

mantepse commented 5 years ago

Branch: u/mantepse/py3__fix_fricas_interface

mantepse commented 5 years ago

Author: Martin Rubey

mantepse commented 5 years ago

Commit: ec66106

mantepse commented 5 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+python3 likes modifying a dict one is iterating over even less than python2.
mantepse commented 5 years ago

Dependencies: #25899

mantepse commented 5 years ago

New commits:

ec66106fix treatment of rootOf expressions for python3
slel commented 5 years ago
comment:3

Might this help with #17908 and #25905? Should there also be a register_symbol for abs?

mantepse commented 5 years ago
comment:4
slel commented 5 years ago
comment:5

Replying to @mantepse:

... thank you for notifying me! ...

You can query Trac for all tickets whose summary contains "fricas":

mantepse commented 5 years ago

Changed keywords from none to FriCAS

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

9a6ec3dMerge branch 'develop' of git://trac.sagemath.org/sage into t/27268/py3__fix_fricas_interface
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from ec66106 to 9a6ec3d

tscrim commented 5 years ago

Reviewer: Travis Scrimshaw

tscrim commented 5 years ago
comment:8

Actually, the problem is this line: (var, poly) = rootOf.items()[i]. The result of items() is a view, which does not support indexing (in a way, think of it like an iterator). However, since your changes work and the code looks cleaner, positive review.

mantepse commented 5 years ago
comment:9

Replying to @tscrim:

Actually, the problem is this line: (var, poly) = rootOf.items()[i].

Yes, the error message and trace did show that.

The result of items() is a view, which does not support indexing (in a way, think of it like an iterator). However, since your changes work and the code looks cleaner, positive review.

General question: should I modify the ticket description once I know the root of the problem? The downside is that the history how the bugfix developed is sometimes not as easy to understand anymore.

tscrim commented 5 years ago
comment:10

Yes, in this case it probably is better to modify it so that people have an easier time seeing the issue (rather than reading the comments).

mantepse commented 5 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-python3 likes modifying a dict one is iterating over even less than python2.
+In python3, `dict.items` cannot be indexed.  The processing of `rootOf` expression assumed this, so the design pattern must be changed.
vbraun commented 5 years ago

Changed branch from u/mantepse/py3__fix_fricas_interface to 9a6ec3d