Opened 9 months ago
Closed 8 months ago
#27450 closed enhancement (fixed)
some refactoring in plot3d
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.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
 Branch set to u/chapoton/27450
 Commit set to b05f6818e2548b5dc179b728fef9b57bcd2d5f50
 Status changed from new to needs_review
comment:2 Changed 9 months ago by
 Status changed from needs_review to needs_work
comment:3 Changed 9 months ago by
 Commit changed from b05f6818e2548b5dc179b728fef9b57bcd2d5f50 to e5bcc913a9d0445c9f48f3e26c253c1ffbbc5e9d
comment:4 Changed 9 months ago by
 Status changed from needs_work to needs_review
comment:5 Changed 9 months ago by
 Status changed from needs_review to needs_work
comment:6 Changed 9 months ago by
 Milestone changed from sage8.7 to sage8.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
 Commit changed from e5bcc913a9d0445c9f48f3e26c253c1ffbbc5e9d to 6131b9773b5cccc4d8fb78dee03c5bb853f0e752
comment:8 Changed 8 months ago by
 Status changed from needs_work to needs_review
comment:9 Changed 8 months ago by
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
 Commit changed from 6131b9773b5cccc4d8fb78dee03c5bb853f0e752 to e2ea4ef2251fa062dd3bcfbdffe69cc79f424f36
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e2ea4ef  some refactoring in parametric_plot3d (small scale changes)

comment:11 Changed 8 months ago by
ok, deprecation done as required, thx
comment:12 Changed 8 months ago by
 Commit changed from e2ea4ef2251fa062dd3bcfbdffe69cc79f424f36 to 9b40e2aec50c40a21eed89a2da6311aedd47a443
Branch pushed to git repo; I updated commit sha1. New commits:
9b40e2a  trac 27450 remove unused declarations

comment:13 Changed 8 months ago by
 Status changed from needs_review to needs_work
still one failing doctest, see bots
comment:14 followup: ↓ 17 Changed 8 months ago by
 Commit changed from 9b40e2aec50c40a21eed89a2da6311aedd47a443 to a703dcea09bb7a5993e74c7cf79b49ddb5cd13a1
Branch pushed to git repo; I updated commit sha1. New commits:
a703dce  fix doctest in plot3d

comment:15 Changed 8 months ago by
 Status changed from needs_work to needs_review
comment:16 Changed 8 months ago by
green bot, please review
comment:17 in reply to: ↑ 14 Changed 8 months ago by
comment:18 followup: ↓ 23 Changed 8 months ago by
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
Erik ? do you want me to make it more clear that the doctest was unchanged ?
comment:21 Changed 8 months ago by
Only comment from me is if not(foo and bar):
> if not (foo and bar):
.
comment:22 Changed 8 months ago by
 Commit changed from a703dcea09bb7a5993e74c7cf79b49ddb5cd13a1 to acfcde7173c5a343be3848ae86e425783024c320
comment:23 in reply to: ↑ 18 Changed 8 months ago by
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
 Commit changed from acfcde7173c5a343be3848ae86e425783024c320 to 0a6b6740cf10695326e1d153dc919ecf336f0ff5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0a6b674  some refactoring in parametric_plot3d (small scale changes)

comment:25 Changed 8 months ago by
I have squashed my commits.
The answer is "no change".
comment:26 Changed 8 months ago by
and the bot is green again
comment:27 Changed 8 months ago by
 Reviewers set to Erik Bray, Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM.
comment:28 Changed 8 months ago by
 Branch changed from u/chapoton/27450 to 0a6b6740cf10695326e1d153dc919ecf336f0ff5
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
some refactoring in parametric_plot3d (small scale changes)