make Graphics class inheritable and some clean ups

[ppurka]

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​.

[ppurka]

apply this first to devel/sage

[kcrisman]

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.

[ppurka]

Apply this first to devel/sage

[ppurka]

next, apply this to devel/sage

[ppurka]

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
   

2. I removed the backslash and replaced it with simple brackets
3. 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.
4. 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).

[ppurka]

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

[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.

[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.

[ppurka]

third patch which reorders some arguments

[ppurka]

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

[ppurka]

patchbot: apply trac_12974-fix_graphics_attributes.patch​ trac_12974-refactor.patch​ trac_12974-reorder_some_arguments.patch​ trac_12974-whitespace_cleanup.patch​

[kcrisman]

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 :)

[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 !!!

[ppurka]

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

[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)

[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...

[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.

[kcrisman]

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

[ppurka]

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).

[jdemeyer]

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.

[jdemeyer]

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

[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

[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.

[kcrisman]

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.

[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.

[kcrisman]

Patchbot:

Apply 12974_flat.patch​ and trac_12974-colorfix.patch​.

[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.

[kcrisman]

Okay, all is well! Reinstating positive review.

[kcrisman]

By the way, only downside here is that now Jeroen looks like the author of the patch in the history :)

[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...) :)