Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sage.plot.plot3d.texture minor refactoring #27593

Closed
embray opened this issue Apr 2, 2019 · 18 comments
Closed

sage.plot.plot3d.texture minor refactoring #27593

embray opened this issue Apr 2, 2019 · 18 comments

Comments

@embray
Copy link
Contributor

embray commented Apr 2, 2019

The module sage.plot.plot3d.texture defines a Texture_class class, and a Texture function that acts as its alternate constructor, along with an is_Texture function, à la the old scheme used elsewhere in Sage.

As discussed elsewhere (e.g. in #12824) the is_Texture function should be deprecated. The Texture function and Texture_class class can be merged into a single Texture class with use of a __classcall__ method.

CC: @egourgoulhon @paulmasson @slel @tscrim

Component: graphics

Keywords: texture

Author: Erik Bray

Branch/Commit: 604398e

Reviewer: Travis Scrimshaw

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

@embray embray added this to the sage-8.8 milestone Apr 2, 2019
@embray
Copy link
Contributor Author

embray commented Apr 3, 2019

Branch: u/embray/plot3d/texture-refactoring

@embray
Copy link
Contributor Author

embray commented Apr 3, 2019

Author: Erik Bray

@embray
Copy link
Contributor Author

embray commented Apr 3, 2019

comment:1

I do think the use of __classcall__ is a bit overkill here, since it's basically just doing some argument pre-processing that could probably just as well be done in the __init__. I thought maybe it would also cache some textures but apparently it doesn't even do that.

That said, this branch preserves the existing functionality, and I think in a better way than having a separate Texture_class and Texture factory function.


New commits:

23f043bTrac #27593: Minor refactoring of Texture+Texture_class into a single Texture class

@embray
Copy link
Contributor Author

embray commented Apr 3, 2019

Commit: 23f043b

@embray
Copy link
Contributor Author

embray commented Jul 3, 2019

comment:2

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@slel
Copy link
Member

slel commented Aug 29, 2019

Changed keywords from none to texture

@embray
Copy link
Contributor Author

embray commented Dec 30, 2019

comment:4

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-8.9, sage-9.1 Dec 30, 2019
@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 14, 2020

comment:5

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@fchapoton
Copy link
Contributor

Changed branch from u/embray/plot3d/texture-refactoring to public/ticket/27593

@fchapoton
Copy link
Contributor

comment:6

rebased, refreshed


New commits:

d39f55eMerge branch 'u/embray/plot3d/texture-refactoring' in 9.2.b8

@fchapoton
Copy link
Contributor

Changed commit from 23f043b to d39f55e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

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

604398efix use of is_Texture in plot3d base

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

Changed commit from d39f55e to 604398e

@fchapoton
Copy link
Contributor

comment:8

green bot please review

@tscrim
Copy link
Collaborator

tscrim commented Aug 17, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 17, 2020

comment:9

LGTM.

@embray
Copy link
Contributor Author

embray commented Aug 19, 2020

comment:10

Thanks for rebasing.

@vbraun
Copy link
Member

vbraun commented Aug 23, 2020

Changed branch from public/ticket/27593 to 604398e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants