#28461 closed enhancement (fixed)

pep cleanup for the quivers folder

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-8.9
Component: algebra Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: e7695d3 (Commits) Commit: e7695d30b52fe0d3db7c2b7dbc51980b39e4a308
Dependencies: Stopgaps:

Description

mainly cosmetic changes in code presentation

Change History (7)

comment:1 Changed 10 months ago by chapoton

  • Branch set to u/chapoton/28461
  • Cc tscrim added
  • Commit set to 08cc7dc00f3ee8bf8910031206e132e2d8a19769
  • Status changed from new to needs_review

New commits:

08cc7dcpep cleanup for the quivers folder

comment:2 Changed 10 months ago by chapoton

green bot, please review

comment:3 Changed 10 months ago by tscrim

Is there a reason why you didn't also change this to a dict comprehension?

-        elems = dict((v, self._left_action_mats[edge][v]*element._elems[v]) for v in self._quiver)
+        elems = dict((v, self._left_action_mats[edge][v]*element._elems[v])
+                     for v in self._quiver)

In this change:

-        result = parent()   # this must not be the cached parent.zero(),
-                             # since otherwise it gets changed in place!!
+        result = parent()
+        # this must not be the cached parent.zero(),
+        # since otherwise it gets changed in place!!

I think it would be better for the comment to go first. Also, in principle, the result of parent() could use the cached zero method since we assume elements are immutable. So I think it would be better (and faster) to explicitly construct the zero element.

comment:4 Changed 10 months ago by git

  • Commit changed from 08cc7dc00f3ee8bf8910031206e132e2d8a19769 to e7695d30b52fe0d3db7c2b7dbc51980b39e4a308

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

a433abdMerge branch 'u/chapoton/28461' in 8.9.b9
e7695d3trac 28461 fix some details

comment:5 follow-up: Changed 10 months ago by chapoton

I have made the trivial changes that you suggested.

Concerning the zero, it seems that there is really a problem. This looks serious enough that it should be kept for a dedicated ticket. Moreover, I am not sure that the coercion framework is used correctly, as this happens inside a double underscore method __mul__.

comment:6 in reply to: ↑ 5 Changed 10 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Replying to chapoton:

I have made the trivial changes that you suggested.

Thank you.

Concerning the zero, it seems that there is really a problem. This looks serious enough that it should be kept for a dedicated ticket. Moreover, I am not sure that the coercion framework is used correctly, as this happens inside a double underscore method __mul__.

Sounds good. cc me on that and I can do the review, or I can make the change tomorrow if you have not started on it already.

comment:7 Changed 10 months ago by vbraun

  • Branch changed from u/chapoton/28461 to e7695d30b52fe0d3db7c2b7dbc51980b39e4a308
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.