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

Introduce decorator: wrap and autowrap #7

Merged
merged 10 commits into from
Dec 19, 2018
Merged

Introduce decorator: wrap and autowrap #7

merged 10 commits into from
Dec 19, 2018

Conversation

wookayin
Copy link
Owner

A prototype implementation of #3.

  • tfplot.wrap: It can be used as a decorator as well as with a normal function call.
  • tfplot.autowrap: Similar to wrap, but supports additional features (e.g. automatic injection of fig, ax, etc.)

This PR is for reviewing.

This commit adds a minimal implementation of @tfplot.wrapper.as_op
decorator which can wrap a python function to TensorFlow operation
(factory).

One additional feature is auto-injection of `fig`, `ax` instance
to avoid manual invocation of `tfplot.subplots()` if appropriate.

Batch support and agglomeration  wrapper will be added later.
plus some style improvement on the unit tests
The existing function tfplot.wrap() can actually be turned into
a python decorator providing the same functionality. To this end,
we use biwrap to support two usage of tfplot.wrap():

- As a function: tfplot.wrap(plotter, option='..')
- As a decorator:
  ```python
  @tfplot.wrap
  def plotter(tensor_val, option='..'):
    # ...
    return fig
  ```

to create a factory of plotting Tensors.

In the same vein, a new decorator @tfplot.autowrap is added.
It provides with additional features such as automatic injecting
matplotlib Figure and Axes objects if specified and handling of
return values (e.g. return ax directly). This will make most of
use cases easier.
Python2 may raise SyntaxError on collecting test suites
that contains a function with keyword-only arguments.

To work around this, we use a exec trick to dynamically define
a decorator-wrapped function. Argspec is handled as well.
@wookayin wookayin merged commit 04014dd into master Dec 19, 2018
wookayin added a commit that referenced this pull request Dec 19, 2018
As a result, `tfplot.autowrap()` can totally replace
`tfplot.wrap_axesplot()` (which now shall be deprecated).

Follow-up of #7.
wookayin added a commit that referenced this pull request Dec 25, 2018
wookayin added a commit that referenced this pull request Dec 25, 2018
Additional kwargs of autwrap() should be passed through to the
python function when executed inside a TF graph.

Follow-up of #7.
Copy link

@ferrine ferrine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this code is fine, and well documented. I do not have big concerns on design as it seems to be quite agnostic to the function used



@biwrap
def wrap(plot_func=REQUIRED, _sentinel=None,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using keyword only arguments? Compatibility?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to support python 2.7 for the time being (See TF's cross entropy loss function).

@functools.wraps(plot_func)
def _wrapped_plot_fn(*args, **kwargs_call):
# (1) auto-inject fig, ax
if fig_ax_mode:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit hard to follow

(as a decorator or with normal function call), but provides with additional
features such as auto-creating matplotlib figures.

- (``fig``, ``ax``) matplotlib objects are automatically created and
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, well documented

@wookayin
Copy link
Owner Author

@ferrine Thanks for your comments!

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

Successfully merging this pull request may close these issues.

2 participants