Opened 10 years ago
Closed 10 years ago
#12974 closed enhancement (fixed)
make Graphics class inheritable and some clean ups
Reported by: | ppurka | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-5.2 |
Component: | graphics | Keywords: | sd40.5 |
Cc: | jason | Merged in: | sage-5.2.beta0 |
Authors: | Punarbasu Purkayastha | Reviewers: | Karl-Dieter Crisman, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12810, #12605 | Stopgaps: |
Description (last modified by )
The first patch makes all the .__attribute
into ._attribute
so that the Graphics
class can be inherited in the future. This is desirable since all the important functions are present in this class. One will not lose anything in terms of existing functionality.
The second patch does refactoring of one big bunch of code which is present twice in Graphics().matplotlib()
.
The third and fourth patches are more of a 'cleanup' nature.
Since then the patches have been cleaned up themselves in various ways, see discussion.
Apply 12974_flat.patch and trac_12974-colorfix.patch.
Attachments (11)
Change History (42)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
In general this looks pretty good, good work on underscores and the refactor looks ok. The Sage Days 40.5 crowd says "yes!" to making double underscores single underscores.
- Minor but should be done: get rid of backslashes ending lines. Suggestion is to just enclose things in parentheses for splitting lines.
- You need to doctest your new matplotlib tick formatter.
- I'm not sure why you alphabetized some of the keywords in various places. It might make more sense to keep ones together that are related - e.g.,
vgridlinesstyle
andhgridlinesstyle
should be next to each other. I don't think this was an optimal change.
comment:3 Changed 10 years ago by
- Keywords sd40.5 added
comment:4 follow-up: ↓ 20 Changed 10 years ago by
- Description modified (diff)
- I added the new file, where I don't reorder the arguments. Also, I ran the following sed expression (in zsh) to cleanup trailing whitespaces from
sage/plot
....rc0/devel/sage/sage/plot» for i in **/*.py; do sed -i -e 's/[[:space:]]\+$//' "$i"; done
- I removed the backslash and replaced it with simple brackets
- I had also doctested the new matplotlib function, but it seems that went into the log plot code. Sorry for the mixup. The updated patch (the second patch) contains the doctest now.
- From the doctests in #4529, I found that there were other functions outside of
sage/plot
that were using attributes ofGraphics
as_Graphics__object
, etc. All these are fixed now (in the first patch).
comment:5 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Added a third patch which reorders the arguments, but keeps them grouped.
comment:6 Changed 10 years ago by
I like all of this (obviously only did a spot-check of the whitespace file but it seems fine. I do think that we can't reorder the arguments to the function/method matplotlib
, as that would be a change in the API. The other order changes don't matter because they are dicts or just initialization, but this one I don't think we can change, even with the relatively slight possibility that someone actually relies on the keyword order.
But I think you can still do the same idea of putting the comments for clarity - I like very much what you did with this overall. Fix the matplotlib
thing, and in the meantime I'll run doctests and look at some of the plots.
comment:7 Changed 10 years ago by
I think positive review to the changes in the first and second patch. It would have been really helpful not to put the matplotlib refactoring in the same patch as the whitespace, because Trac won't preview it, and that means a lot of tedious searching. But I think that's ok now.
comment:8 Changed 10 years ago by
- Status changed from needs_review to needs_work
- Work issues set to keep same ordering of arguments
Changed 10 years ago by
Changed 10 years ago by
comment:9 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues keep same ordering of arguments deleted
Sorry for the mixup of the whitespace and refactoring patches - I wasn't thinking clearly. I have uploaded the new patches. The whitespace one is completely on its own now. The patches are
- trac_12974-fix_graphics_attributes.patch - makes the change . to ._
- trac_12974-refactor.patch - refactoring the tick formatter in Graphics.matplotlib
- trac_12974-reorder_some_arguments.patch - does the reordering of the arguments in the dicts. matplotlib is untouched now.
- trac_12974-whitespace_cleanup.patch - does the whitespace cleanup via the sed function mentioned in comment:4
comment:10 Changed 10 years ago by
comment:11 Changed 10 years ago by
- Status changed from needs_review to positive_review
This seems to still be fine. Positive review.
Note to release manager - the whitespace cleanup patch might cause some other stuff touched during SD 40.5 to not apply (? I haven't tried) so just don't apply that if it causes problems. I'm sure ppurka won't mind apply his sed script to things after the workshop :)
comment:12 Changed 10 years ago by
Patchbot, listen up! Apply trac_12974-fix_graphics_attributes.patch trac_12974-refactor.patch trac_12974-reorder_some_arguments.patch trac_12974-whitespace_cleanup.patch !!!
comment:13 Changed 10 years ago by
What is this problem with fft in the patchbot output? These patches don't even have the string fft!!
comment:14 Changed 10 years ago by
That patchbot is on the fritz, I guess... wait for arando to get to it, it's running 5.1.beta1 already and so should supersede all other patchbot verdicts 8)
comment:15 Changed 10 years ago by
Not really worried about patchbot, to be honest.
Guess you must have made it home... my flight is delayed by a half hour, so I might as well start rebasing all the stuff I reviewed to 5.1.beta1...
comment:16 Changed 10 years ago by
Well, it would be nice to at least have people pay some attention to the patchbot. If everyone says "ehh, I don't care what the patchbot says", then what's the point of having it?
Yes, I made it back, and David and Doug too, but Jeroen missed his flight, unfortunately - we had a flat tire on the way to the airport and spent almost an hour fixing it for various reasons.
Changed 10 years ago by
Changed 10 years ago by
comment:17 Changed 10 years ago by
- Dependencies set to #12810, #12605
- Description modified (diff)
Okay, this is now "rebased" to depend on #12810 and #12605 (in that order) so that we don't get more messages from the release manager. At least I hope I did this right. Then I'll try to rebase #4529 to that. Not much else to do until the plane gets here...
Apply patches in this order to devel/sage
trac_12974-fix_graphics_attributes-rebase.patch trac_12974-refactor.patch trac_12974-reorder_some_arguments.patch trac_12974-whitespace-rebased.patch
Changed 10 years ago by
comment:18 Changed 10 years ago by
- Description modified (diff)
updated fix attributes patch to address new code in sage-5.1beta1. Changes to the previous patch can be viewed here.
patchbot apply: trac_12974-fix_graphics_attributes-rebase.2.patch trac_12974-refactor.patch trac_12974-reorder_some_arguments.patch trac_12974-whitespace-rebased.patch
(Also ran a make ptestlong
with #4529, #12974, #12810, #12605 and all tests pass).
comment:19 Changed 10 years ago by
- Milestone changed from sage-5.1 to sage-5.2
comment:20 in reply to: ↑ 4 Changed 10 years ago by
- Status changed from positive_review to needs_work
Replying to ppurka:
Also, I ran the following sed expression (in zsh) to cleanup trailing whitespaces from
sage/plot
Please don't do this, it can only lead to merge conflicts. Removing whitespace from parts of the code which you edit is fine (even encouraged), but don't just remove all whitespace shotgun-style.
This needs to be rebased anyway, I'm going to add a new patch.
comment:21 Changed 10 years ago by
- Description modified (diff)
- Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Jeroen Demeyer
- Status changed from needs_work to needs_review
First three patches put in one big patch, the whitespace patch removed.
Changed 10 years ago by
comment:22 Changed 10 years ago by
I tried it on beta5 but it failed a couple of doctests. I will try again in a few days. I am traveling in a couple of hours so I can't install and test on beta6.
The following tests failed: sage -t devel/sage-main/sage/plot/plot3d/base.pyx # 4 doctests failed sage -t devel/sage-main/sage/plot/plot3d/texture.py # 2 doctests faile sage -t devel/sage-main/sage/plot/colors.py # 12 doctests failed ---------------------------------------------------------------------- Timings have been updated. Total time for all tests: 232.5 seconds
comment:23 Changed 10 years ago by
+1 to not having the whitespace patch. There are already huge conflicts with this and the loglog plotting patch, and I think the whitespace patch greatly exaggerates the conflicts.
comment:24 Changed 10 years ago by
- Status changed from needs_review to needs_work
Ok, these are all manifestations of the same thing, exactly.
File "/Users/.../sage-5.1.beta6/local/lib/python/site-packages/sage/plot/colors.py", line 509, in __eq__ return self.__rgb == right.__rgb AttributeError: 'Color' object has no attribute '_Color__rgb'
So there was one other "name-mangling" thing that didn't get picked up, I guess. I'll see if I can track this down fairly easily.
comment:25 Changed 10 years ago by
Changed 10 years ago by
comment:26 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Patchbot:
Apply 12974_flat.patch and trac_12974-colorfix.patch.
comment:27 Changed 10 years ago by
This passes all tests in plot. I'm still going to run full doctests in case we get another thing like the padic one Jeroen caught, but hopefully this is now ready.
comment:28 Changed 10 years ago by
- Status changed from needs_review to positive_review
Okay, all is well! Reinstating positive review.
comment:29 follow-up: ↓ 30 Changed 10 years ago by
By the way, only downside here is that now Jeroen looks like the author of the patch in the history :)
comment:30 in reply to: ↑ 29 Changed 10 years ago by
Replying to kcrisman:
By the way, only downside here is that now Jeroen looks like the author of the patch in the history :)
Which wouldn't be a problem if we used git and pull requests (unless Jeroen rebased the branch...) :)
comment:31 Changed 10 years ago by
- Merged in set to sage-5.2.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
apply this first to devel/sage