search

wxWidgets / Phoenix

wxPython's Project Phoenix. A new implementation of wxPython, better, stronger, faster than he was before.
http://wxpython.org/
2.23k stars 510 forks source link

Additional Refactoring of lib.plot [Part 3: other changes] #117

Closed dougthor42 closed 6 years ago

dougthor42 commented 7 years ago

Update 2017-09-08

I will not be able to finish all the items in the PR 😢

Per @RobinD42's suggestion, I have made issues for all unfinished items so that someone else can complete them.

This PR now only makes the following changes:

All the other items that I wanted to accomplish have their related Issue listed below.

Original PR message

(This PR replaces #112, as I mucked up the git history too much)

This PR will (hopefully) finish off all the changes that I had planned for lib.plot.

It will also include any feedback from Robin and anyone else.

dougthor42 commented 7 years ago

@RobinD42

I'm working on the docs for this, trying to fix the docs showing wrapper(*args, **kwargs) for decorated functions and also updating cross references so that they work correctly

Example: image

However, I'm having trouble building the docs locally so I can't preview the changes to see if they are working. Should I just assume that the changes are OK and then check them after the buildbot releases a snapshot after the merge? Or do you have another option?

The issues I'm having:

Am I missing something in my build flow?

  1. Cloned my repo dougthor42/Phoenix.git
  2. Checked out my branch refactor-lib.plot-part3
  3. git submodule update --init
  4. python build.py dox
  5. python build.py etg
RobinD42 commented 7 years ago

I was looking at the issue with the docs for the deprecated methods a few days ago. It might be due to the commented-out line self._update_docs() in PendingDeprecation but I didn't test that. BTW, there is a wx.deprecated decorator in the core namespace, (however I see now that it isn't included in the documentation! Shame on me.) It's quite a bit more complex than your PendingDeprecation class as it is able to mark classes and properties as deprecated in addition to functions and methods, so if you want to stick with yours that is okay.

The ReST docs for wx.lib are generated with a separate build command, python build.py wxlib. Since it actually imports the files as it is scanning them you need to be sure that there is a built version of Phoenix on the python path. If it's an installed one rather than a built Phoenix in the workspace, then you may need to do some tricks to make sure it is importing wx.lib from your project workspace instead of the installed wx.lib.

The files in _static/images/inheritance are generated by the dot utility from graphviz. Copying them into place is probably an acceptable workaround if you don't have that installed.

dougthor42 commented 7 years ago

Thanks for the info, it allowed me to get the doc builds running locally so I can play around and try to fix things.

Couple of notes for Future Me:

  1. I had to remove the call to demo.run_demo() in __main__.py, as that file would get executed and the demo would pop up. I've gotta come up with another solution for this - perhaps we just don't allow users to call plot as a module (python -m wx.lib.plot)?
  2. Some research [1] suggested adding the call signature as the first line of the docstring on the decorated function. This does not appear to work.
  3. Overriding the call signature in the wx.lib.plot.txt sphinx file is not feasible.
  4. Perhaps switching to a function-based decorator will work...
  5. @functools.wraps(func) decorator should take care of copying __doc__ and __name__.
  6. What about using the decorator package?
  7. Can I use wx.deprecated as a decorator? And would that fix things?
  8. Doc build process:
    1. git clean -d
    2. Make test changes to live wx in .../site-packages/wx/lib/plot
    3. Copy inheritance folder to docs/sphinx/_static/images (Graphvis website is down - can't download)
    4. python build.py wxlib
    5. python build.py sphinx
    6. Check html files.
RobinD42 commented 7 years ago

I had to remove the call to demo.run_demo() in main.py, as that file would get executed and the demo would pop up. I've gotta come up with another solution for this - perhaps we just don't allow users to call plot as a module (python -m wx.lib.plot)?

I had already changed that file in the master branch, see 128b28c8795a629df03f207dd94cbfba50b01f4f

Can I use wx.deprecated as a decorator? And would that fix things?

Yes, and I think so. There is also wx.deprecatedMsg for when you want to use it as a decorator and also have a custom message. It's a little awkward, but works. If you see any improvements that can be made there feel free to propose something. The implementations are here: https://github.com/wxWidgets/Phoenix/blob/master/src/core_ex.py#L39

dougthor42 commented 7 years ago

Decorated Functions + Sphinx

Doesn't look like any decorators will work. I tried switching to function-based decorators and tried using wx.deprecatedMsg to no avail (the method would not even be added to the docs).

So what I've decided to do is simply add a line to the start of every deprecated function which raises PendingDeprecationWarning instead of decorating them:

def GetFontSizeTitle(self):
    """
    Get Title font size (default is 15 point)

    .. deprecated:: Feb 27, 2016

       Use the :attr:`~wx.lib.plot.plotcanvas.PlotCanvas.fontSizeTitle`
       property instead.
    """
    pendingDeprecation('fontSizeTitle')
    return self.fontSizeTitle
def pendingDeprecation(new_func):
    warn_txt = "`{}` is pending deprecation. Please use `{}` instead."
    _warn(warn_txt.format(inspect.stack()[1][3], new_func),
          PendingDeprecationWarning)

The above renders correctly:

image

Properties + Sphinx

The other doc question I had was about properties. Right now, properties are rendered by sphinx as:

image

However, I know that properties can be rendered and that the documentation must be on the getter. I'd like to have the full docstring rendered rather than just two "see X, Y" links which point to the very item that they're under. Is this something that we can change?

A simple docstring on the property would be:

@property
def fontSizeTitle(self):
    """
    The current Title font size in points.

    Use this property to change the size of the plot title. All units are in pts, and the default size is 15pt.

    :getter: Return the size of the plot title.
    :setter: Change the size of the plot title.
    :type:   int or float
    """
    return self._fontSizeTitle
RobinD42 commented 7 years ago

Decorated functions: I thought that I had verified wx.deprecated was working as desired, but I may have looked at the wrong example or something. Your workaround is fine with me.

Properties: Andrea's implementation for generating docs for Python code doesn't actually use Sphinx's module scanner tool, etc. It does all the introspection and docstring extraction itself, in order to be able to generate things the same for those modules as it does for the extension modules. So the issue here is simply that the wxlib build command is not doing anything with the properties other than listing and linking the getter and setter methods. It would be great to have it be able to do more than that. If you're interested in working on enhancing that part of the docs generation, the code to tweak is in the Property class in Phoenix/sphinxtools/librarydescription.py.

RobinD42 commented 7 years ago

I took another look at it and realized that most of what was needed is already there, so I just pushed a change on master that will output the getter's docstring it is has one.

RobinD42 commented 7 years ago

@dougthor42, just FYI I made some minor changes to plot/examples/demo.py on master. See ce00fc06a5f778860f6dbb4db2273b54a813e997

dougthor42 commented 7 years ago

Thanks for the info! Also thanks for making the getter docstring change

And sorry for stalling on this PR. Things have picked back up for me so I haven't had much time recently.

RobinD42 commented 7 years ago

When this PR is finished go ahead and remove the wip label and add the needs-review label. Thanks.

dougthor42 commented 6 years ago

@RobinD42 It's been over a year, I'm so sorry! :sob: I don't think I'll be able to get around to finishing this.

The without this PR, the plot package should work decently well for some of the simpler cases - after all, the goal was never to replicate Matplotlib or pyqtgraph and the like. In my opinion, the biggest issue with not finishing this particular PR are the docs and deprecation notes.

How do you want to proceed? Some options are:

RobinD42 commented 6 years ago

I'll leave it up to you. If the current changes make sense to include we can merge this one, but it would also be nice to not lose track of the remaining todo items in case you or somebody else would like to work on them in the future. Maybe you could create an Issue with those items, links to this series of PRs and maybe a bit of explanation of what you had in mind for the remaining steps if they are not clear.

dougthor42 commented 6 years ago

Great idea. I've gone ahead and made separate issues for each item and updated the original PR message accordingly.

The new issues are:

I've removed the WIP from the title and this is ready for review.

RobinD42 commented 6 years ago

Thanks!

aKummur commented 5 years ago

Does this update allow a plot to have multiple scales like the one in - https://matplotlib.org/gallery/api/two_scales.html ?

dougthor42 commented 5 years ago

TBH I don't remember, haha. My gut tells me no, but let me investigate a bit.

dougthor42 commented 5 years ago

@aKummur I've confirmed: Sadly the plot package does not support multiple scales.

aKummur commented 5 years ago

@dougthor42 Thank you for confirming.