Opened 9 months ago

Closed 8 months ago

#27450 closed enhancement (fixed)

some refactoring in plot3d

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.8
Component: graphics Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Erik Bray, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 0a6b674 (Commits) Commit: 0a6b6740cf10695326e1d153dc919ecf336f0ff5
Dependencies: Stopgaps:

Description

just small changes, no major revamp

One could be smarter when plotting these things, but not now.

Change History (28)

comment:1 Changed 9 months ago by chapoton

  • Branch set to u/chapoton/27450
  • Commit set to b05f6818e2548b5dc179b728fef9b57bcd2d5f50
  • Status changed from new to needs_review

New commits:

b05f681some refactoring in parametric_plot3d (small scale changes)

comment:2 Changed 9 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:3 Changed 9 months ago by git

  • Commit changed from b05f6818e2548b5dc179b728fef9b57bcd2d5f50 to e5bcc913a9d0445c9f48f3e26c253c1ffbbc5e9d

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

c67eb4aMerge branch 'u/chapoton/27450' in 8.7.b7
e5bcc91trying to fix compilation of index_face_set

comment:4 Changed 9 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:5 Changed 9 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:6 Changed 9 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:7 Changed 8 months ago by git

  • Commit changed from e5bcc913a9d0445c9f48f3e26c253c1ffbbc5e9d to 6131b9773b5cccc4d8fb78dee03c5bb853f0e752

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

da404d0Merge branch 'u/chapoton/27450' in 8.8.b0
6131b97trac 27450, fix for plot3d

comment:8 Changed 8 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:9 Changed 8 months ago by embray

Why does this remove

-def len3d(v):
-    """
-    Return the norm of a vector in three dimensions.
-
-    EXAMPLES::
-
-        sage: from sage.plot.plot3d.index_face_set import len3d
-        sage: len3d((1,2,3))
-        3.7416573867739413
-    """
-    return sqrt(v[0] * v[0] + v[1] * v[1] + v[2] * v[2])
-
-

Even if it isn't used anywhere, and probably doesn't belong there (which I agree), it's still a documented, public function, and has to go through a deprecation process. An unfortunate fact of overdocumenting and underuse of private underscored functions.

comment:10 Changed 8 months ago by git

  • Commit changed from 6131b9773b5cccc4d8fb78dee03c5bb853f0e752 to e2ea4ef2251fa062dd3bcfbdffe69cc79f424f36

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e2ea4efsome refactoring in parametric_plot3d (small scale changes)

comment:11 Changed 8 months ago by chapoton

ok, deprecation done as required, thx

comment:12 Changed 8 months ago by git

  • Commit changed from e2ea4ef2251fa062dd3bcfbdffe69cc79f424f36 to 9b40e2aec50c40a21eed89a2da6311aedd47a443

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

9b40e2atrac 27450 remove unused declarations

comment:13 Changed 8 months ago by chapoton

  • Status changed from needs_review to needs_work

still one failing doctest, see bots

comment:14 follow-up: Changed 8 months ago by git

  • Commit changed from 9b40e2aec50c40a21eed89a2da6311aedd47a443 to a703dcea09bb7a5993e74c7cf79b49ddb5cd13a1

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

a703dcefix doctest in plot3d

comment:15 Changed 8 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:16 Changed 8 months ago by chapoton

green bot, please review

comment:17 in reply to: ↑ 14 Changed 8 months ago by embray

Replying to git:

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

a703dcefix doctest in plot3d

Do we know why this test result changed and if that change is expected/acceptable?

comment:18 follow-up: Changed 8 months ago by chapoton

hmm, in fact, it probably did not change. It seems to have changed during some intermediate state of the code. I then put it back, but not perfectly..

comment:19 Changed 8 months ago by chapoton

Erik ? do you want me to make it more clear that the doctest was unchanged ?

comment:20 Changed 8 months ago by chapoton

  • Cc tscrim added

green bot, please review

comment:21 Changed 8 months ago by tscrim

Only comment from me is if not(foo and bar): -> if not (foo and bar):.

comment:22 Changed 8 months ago by git

  • Commit changed from a703dcea09bb7a5993e74c7cf79b49ddb5cd13a1 to acfcde7173c5a343be3848ae86e425783024c320

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

57d28d7Merge branch 'u/chapoton/27450' in 8.8.b2
acfcde7trac 27450 details

comment:23 in reply to: ↑ 18 Changed 8 months ago by embray

Replying to chapoton:

hmm, in fact, it probably did not change. It seems to have changed during some intermediate state of the code. I then put it back, but not perfectly..

I still don't understand. Did the output change or not? It should be easy enough to confirm; and if so then the question is "why?"

comment:24 Changed 8 months ago by git

  • Commit changed from acfcde7173c5a343be3848ae86e425783024c320 to 0a6b6740cf10695326e1d153dc919ecf336f0ff5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0a6b674some refactoring in parametric_plot3d (small scale changes)

comment:25 Changed 8 months ago by chapoton

I have squashed my commits.

The answer is "no change".

comment:26 Changed 8 months ago by chapoton

and the bot is green again

comment:27 Changed 8 months ago by tscrim

  • Reviewers set to Erik Bray, Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:28 Changed 8 months ago by vbraun

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