Opened 10 years ago

Closed 10 years ago

#12974 closed enhancement (fixed)

make Graphics class inheritable and some clean ups

Reported by: Punarbasu Purkayastha Owned by: jason, was
Priority: major Milestone: sage-5.2
Component: graphics Keywords: sd40.5
Cc: Jason Grout 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:

Status badges

Description (last modified by Karl-Dieter Crisman)

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)

trac_12974-fix_graphics_attributes_and_reorder_args.patch (26.8 KB) - added by Punarbasu Purkayastha 10 years ago.
apply this first to devel/sage
trac_12974-fix_graphics_attributes.patch (24.6 KB) - added by Punarbasu Purkayastha 10 years ago.
Apply this first to devel/sage
trac_12974-refactor_and_whitespace_cleanups.patch (275.0 KB) - added by Punarbasu Purkayastha 10 years ago.
next, apply this to devel/sage
trac_12974-refactor.patch (15.4 KB) - added by Punarbasu Purkayastha 10 years ago.
trac_12974-reorder_some_arguments.patch (4.3 KB) - added by Punarbasu Purkayastha 10 years ago.
third patch which reorders some arguments
trac_12974-whitespace_cleanup.patch (261.1 KB) - added by Punarbasu Purkayastha 10 years ago.
trac_12974-whitespace-rebased.patch (260.5 KB) - added by Karl-Dieter Crisman 10 years ago.
trac_12974-fix_graphics_attributes-rebase.patch (24.7 KB) - added by Karl-Dieter Crisman 10 years ago.
trac_12974-fix_graphics_attributes-rebase.2.patch (26.1 KB) - added by Punarbasu Purkayastha 10 years ago.
12974_flat.patch (45.0 KB) - added by Jeroen Demeyer 10 years ago.
trac_12974-colorfix.patch (821 bytes) - added by Karl-Dieter Crisman 10 years ago.

Download all attachments as: .zip

Change History (42)

Changed 10 years ago by Punarbasu Purkayastha

apply this first to devel/sage

comment:1 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Status: newneeds_review

comment:2 Changed 10 years ago by Karl-Dieter Crisman

Authors: Punarbasu Purkayastha
Reviewers: Karl-Dieter Crisman
Status: needs_reviewneeds_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 and hgridlinesstyle should be next to each other. I don't think this was an optimal change.

comment:3 Changed 10 years ago by Karl-Dieter Crisman

Keywords: sd40.5 added

Changed 10 years ago by Punarbasu Purkayastha

Apply this first to devel/sage

Changed 10 years ago by Punarbasu Purkayastha

next, apply this to devel/sage

comment:4 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
  1. 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
    
  1. I removed the backslash and replaced it with simple brackets
  2. 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.
  3. From the doctests in #4529, I found that there were other functions outside of sage/plot that were using attributes of Graphics as _Graphics__object, etc. All these are fixed now (in the first patch).

comment:5 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Status: needs_workneeds_review

Added a third patch which reorders the arguments, but keeps them grouped.

comment:6 Changed 10 years ago by Karl-Dieter Crisman

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 Karl-Dieter Crisman

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 Karl-Dieter Crisman

Status: needs_reviewneeds_work
Work issues: keep same ordering of arguments

Changed 10 years ago by Punarbasu Purkayastha

Attachment: trac_12974-refactor.patch added

Changed 10 years ago by Punarbasu Purkayastha

third patch which reorders some arguments

Changed 10 years ago by Punarbasu Purkayastha

comment:9 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Status: needs_workneeds_review
Work issues: keep same ordering of arguments

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

  1. trac_12974-fix_graphics_attributes.patch - makes the change . to ._
  2. trac_12974-refactor.patch - refactoring the tick formatter in Graphics.matplotlib
  3. trac_12974-reorder_some_arguments.patch - does the reordering of the arguments in the dicts. matplotlib is untouched now.
  4. trac_12974-whitespace_cleanup.patch - does the whitespace cleanup via the sed function mentioned in comment:4

comment:11 Changed 10 years ago by Karl-Dieter Crisman

Status: needs_reviewpositive_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 Keshav Kini

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 Punarbasu Purkayastha

What is this problem with fft in the patch bot output? These patches don't even have the string fft!!

Last edited 10 years ago by Punarbasu Purkayastha (previous) (diff)

comment:14 Changed 10 years ago by Keshav Kini

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 Karl-Dieter Crisman

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 Keshav Kini

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 Karl-Dieter Crisman

Changed 10 years ago by Karl-Dieter Crisman

comment:17 Changed 10 years ago by Karl-Dieter Crisman

Dependencies: #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 Punarbasu Purkayastha

comment:18 Changed 10 years ago by Punarbasu Purkayastha

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 Jeroen Demeyer

Milestone: sage-5.1sage-5.2

comment:20 in reply to:  4 Changed 10 years ago by Jeroen Demeyer

Status: positive_reviewneeds_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 Jeroen Demeyer

Description: modified (diff)
Reviewers: Karl-Dieter CrismanKarl-Dieter Crisman, Jeroen Demeyer
Status: needs_workneeds_review

First three patches put in one big patch, the whitespace patch removed.

Changed 10 years ago by Jeroen Demeyer

Attachment: 12974_flat.patch added

comment:22 Changed 10 years ago by Punarbasu Purkayastha

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 Jason Grout

+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 Karl-Dieter Crisman

Status: needs_reviewneeds_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 Karl-Dieter Crisman

Ok, positive review on the flattened patch and its few additions. Now I'll look at the colors thing.

Yup, this is all fallout from #11383 and #12999, which didn't know about this patch. The patch is a three-character change, coming up.

Changed 10 years ago by Karl-Dieter Crisman

Attachment: trac_12974-colorfix.patch added

comment:26 Changed 10 years ago by Karl-Dieter Crisman

Description: modified (diff)
Status: needs_workneeds_review

comment:27 Changed 10 years ago by Karl-Dieter Crisman

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 Karl-Dieter Crisman

Status: needs_reviewpositive_review

Okay, all is well! Reinstating positive review.

comment:29 Changed 10 years ago by Karl-Dieter Crisman

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 Jason Grout

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 Jeroen Demeyer

Merged in: sage-5.2.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.