Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Sep 9, 2017
Merged

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

merged 7 commits into from
Sep 9, 2017

Conversation

dougthor42
Copy link
Contributor

@dougthor42 dougthor42 commented Jul 11, 2016

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:

  • Add an option to draw ticks outside to plot rather than only inside.
  • Fix instances of wrapper(*args, **kwargs) in built docs
  • Updates docs
  • Addes additional "Ticks" demos
  • Miscellaneous cleanup

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
Copy link
Contributor Author

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

  • no static assets. Fixed by downloading the most recent docs and copying the _static/images/inheritance folder from them into my working dir.
  • python build.py etg builds the main docs but nothing from wx.lib.
  • python build.py sphinx builds the main docs but nothing from wx.lib.
  • running make html from the docs/sphinx/ director also doesn't built the wx.lib docs.

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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 128b28c

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@dougthor42
Copy link
Contributor Author

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.

+ added deprecation note to docstring of PolyPoint.setLogScale
+ Replaced PendingDeprecation decorator with function.
  + This fixes the documentation having `wrapper(*args, **kwargs)`
+ Fixed some minor errors due to package conversion
+ Updated sphinx x-refs to reflect new package structure.
+ Fixed typo in top-side ticks.
+ Changed default ticks to negative (extent out of plot rather an into plot).
+ Fixed tickLength setter - was setting unused self._tickWitch value
+ Added tickLengthPrinterScale property
+ axis values and labels should no longer overlap if tick lengths are negative.
@RobinD42
Copy link
Member

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

@dougthor42
Copy link
Contributor Author

@RobinD42 It's been over a year, I'm so sorry! 😭 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:

  • Merge as-is, ignoring all the proposed improvements in the PR note.
  • Close this PR and open a new one dedicated to only the docs and deprecation notes.
  • Close this PR and not merge anything.
  • Cherry-pick some commits
  • Other

@RobinD42
Copy link
Member

RobinD42 commented Sep 8, 2017

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 dougthor42 changed the title [WIP] Additional Refactoring of lib.plot [Part 3: other changes] Additional Refactoring of lib.plot [Part 3: other changes] Sep 8, 2017
@RobinD42 RobinD42 added needs-review and removed wip labels Sep 8, 2017
@RobinD42
Copy link
Member

RobinD42 commented Sep 8, 2017

Thanks!

@RobinD42 RobinD42 merged commit d97731e into wxWidgets:master Sep 9, 2017
@dougthor42 dougthor42 deleted the refactor-lib.plot-part3 branch September 12, 2017 23:23
@aKummur
Copy link

aKummur commented Sep 21, 2018

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

@dougthor42
Copy link
Contributor Author

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

@dougthor42
Copy link
Contributor Author

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

@aKummur
Copy link

aKummur commented Sep 24, 2018

@dougthor42 Thank you for confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants