Opened 9 years ago

Closed 9 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:

Status badges

Description (last modified by kcrisman)

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 ppurka 9 years ago.
apply this first to devel/sage
trac_12974-fix_graphics_attributes.patch (24.6 KB) - added by ppurka 9 years ago.
Apply this first to devel/sage
trac_12974-refactor_and_whitespace_cleanups.patch (275.0 KB) - added by ppurka 9 years ago.
next, apply this to devel/sage
trac_12974-refactor.patch (15.4 KB) - added by ppurka 9 years ago.
trac_12974-reorder_some_arguments.patch (4.3 KB) - added by ppurka 9 years ago.
third patch which reorders some arguments
trac_12974-whitespace_cleanup.patch (261.1 KB) - added by ppurka 9 years ago.
trac_12974-whitespace-rebased.patch (260.5 KB) - added by kcrisman 9 years ago.
trac_12974-fix_graphics_attributes-rebase.patch (24.7 KB) - added by kcrisman 9 years ago.
trac_12974-fix_graphics_attributes-rebase.2.patch (26.1 KB) - added by ppurka 9 years ago.
12974_flat.patch (45.0 KB) - added by jdemeyer 9 years ago.
trac_12974-colorfix.patch (821 bytes) - added by kcrisman 9 years ago.

Download all attachments as: .zip

Change History (42)

Changed 9 years ago by ppurka

apply this first to devel/sage

comment:1 Changed 9 years ago by ppurka

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by kcrisman

  • Authors set to Punarbasu Purkayastha
  • 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 and hgridlinesstyle should be next to each other. I don't think this was an optimal change.

comment:3 Changed 9 years ago by kcrisman

  • Keywords sd40.5 added

Changed 9 years ago by ppurka

Apply this first to devel/sage

Changed 9 years ago by ppurka

next, apply this to devel/sage

comment:4 follow-up: Changed 9 years ago by ppurka

  • 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 9 years ago by ppurka

  • 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 9 years ago by kcrisman

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 9 years ago by kcrisman

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 9 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • Work issues set to keep same ordering of arguments

Changed 9 years ago by ppurka

Changed 9 years ago by ppurka

third patch which reorders some arguments

Changed 9 years ago by ppurka

comment:9 Changed 9 years ago by ppurka

  • 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

  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 9 years ago by kcrisman

  • 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 9 years ago by 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 9 years ago by ppurka

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

Last edited 9 years ago by ppurka (previous) (diff)

comment:14 Changed 9 years ago by 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 9 years ago by kcrisman

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 9 years ago by 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 9 years ago by kcrisman

comment:17 Changed 9 years ago by kcrisman

  • 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

comment:18 Changed 9 years ago by ppurka

  • 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 9 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:20 in reply to: ↑ 4 Changed 9 years ago by jdemeyer

  • 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 9 years ago by jdemeyer

  • 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 9 years ago by jdemeyer

comment:22 Changed 9 years ago by ppurka

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 9 years ago by jason

+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 9 years ago by kcrisman

  • 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 9 years ago by kcrisman

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 9 years ago by kcrisman

comment:26 Changed 9 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:27 Changed 9 years ago by kcrisman

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 9 years ago by kcrisman

  • Status changed from needs_review to positive_review

Okay, all is well! Reinstating positive review.

comment:29 follow-up: Changed 9 years ago by kcrisman

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 9 years ago by jason

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 9 years ago by jdemeyer

  • Merged in set to sage-5.2.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.