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

Deprecation, consistency and docstrings for core methods #3128

Merged
merged 28 commits into from Nov 27, 2018

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented Nov 3, 2018

This PR is an attempt to make the core API more consistent by ensuring that method signatures are consistent, deprecating methods which are not useful or duplicative and add consistent docstrings throughout.

As a first step I added a config variable called future_deprecations which guards all forward going deprecation warnings. A user could enable this flag to see if they are affected by any of the deprecations

Newly deprecated methods

These will go through a whole deprecation cycle, i.e. we will announce them as deprecated in the release notes, then disable the future_deprecations conditional in the next release so the warnings are raised and finally remove these methods. In all likelihood these methods will start raising warnings in 1.12 and be fully removed in 1.0.

  • .table: This method exists on all elements and NdMapping types, despite the fact that a tabular representation makes little sense for many types of data, if we decide to replace this method we should instead add a dataset method which concatenates all types of data appropriately instead of "tabularizing" it. However HoloMap.collapse already offers this capability and in the simple case you can simple cast the Element to a Table so there is not much gained by adding a new method. However this method is used a fair bit throughout the docs so I think it has to go through a longer deprecation cycle. We may also have to consider moving collapse onto UniformNdMapping to allow collapsing types other than a HoloMap.
  • .mapping: This exists on all elements and returns the horrible NdElement based dictionary format which we have removed a long time ago.
  • ItemTable.values: This existed for consistency with our old NdColumns class which has been replaced with Dataset long ago. It's not useful and should be removed. It also means that we might consider renaming dimension_values to values so removing clashes like this now will help us in the long run.
  • HoloMap/DynamicMap .split_overlay: This method is really not very useful to a user so I've decided to make it private
  • Overlay.collapse: We should make collapse an operation that can be applied to all kinds of objects, the Overlay.collapse implementation was hugely outdated and will not work for the majority of types
  • Element.collapse_data: This was a classmethod to perform collapse operations which has slowly been getting replaced with an Interface based implementation
  • Dimension.__call__: Was replaced with Dimension.clone but we never added a warning.
  • ViewableTree.from_values: Old method which was required to construct tree from list of items, this is now supported by the standard constructor
  • ViewableTree.regroup: Recomputes groups for items in ViewableTree which is highly inefficient and very rarely useful, also equivalent functionality achievable with hv.Layout(layout.relabel(group='New Group').values()).

Methods made more consistent

There are still a number of data conversion methods that have some usefulness and are used throughout the code and therefore probably require a longer deprecation cycle if we were to decide to remove them:

  • Element.columns: Returns an OrderedDict of values along each dimensions. The main issue here is that this will flatten gridded and geometry data into a flat column, which is of limited utility. It may be nice to replace this with a method that returns the native dictionary format for each different type of data, columnar, geometry and gridded but we would have to find a different name.
  • Element.array: Returns an ndarray of values along each dimension again in a columnar format. This method is used a fair bit throughout the code and we probably shouldn't remove it.
  • Element.dframe: Returns a pandas dataframe of the values along each dimension, once again in a columnar format. Again this is frequently used so I've given it a good docstring, gave it a consistent signature throughout and allowed key dimensions to become dataframe indexes.
  • Element.sample: Element.sample and HoloMap.sample now accept the same signature (fixes #298)
  • .hist: The .hist implementation had 4 completely different signatures which are now consistent (fixes #1728)

Methods we should consider introducing as replacements

  • UniformNdMapping.dataset: As a replacement for .table we could provide a way method to collapse UniformNdMapping types down to a single Dataset.

Old deprecations

A number of items have been deprecated for a long time but were never removed

  • ElementOperation removed (was replaced with Operation)
  • .to mdims argument no longer supported
  • Remove PositionX, PositionY and PositionXY (were renamed to PointerX/Y)
  • Layout.display used to control display modes for Layout but removed a long time ago

Element hierarchies

Addressing #143

This PR adjusts the inheritance hierarchies, introducing a new category of elements with Geometry as its baseclass.

There are now the following general classes of elements:

  • Charts: These elements have independent and dependent variables corresponding to the key and value dimensions respectively (e.g. Curve, Scatter, Bars, Histogram etc.)
  • Geometries: Geometries represent coordinates in a 2D coordinate system rendered as a number of different shapes (e.g. Points, VectorField, Path, Contours, Polygons etc.)
  • Rasters: These elements represent a discretely sampled grid of an underlying continuous coordinate system (e.g. Raster, Image, RGB, HSV, QuadMesh)
  • Statistics: Subtype of charts where the dependent variables are summary statistics computed from the data
  • Graphs: Connected nodes and edges (e.g. Graph, Chord, Sankey)

Docstrings

  • Partially address #2322 by adding docstrings for most methods on core classes

Classes that have had docstrings added:

  • core/dimension.py
    • LabelledData
    • Dimensioned
  • core/element.py
    • Element
  • core/data/__init__.py
    • Dataset
  • core/layout.py
    • AdjointLayout
    • Layout
    • NdLayout
  • core/ndmapping.py
    • MultiDimensionalMapping
    • NdMapping
    • UniformNdMapping
  • core/overlay.py
    • Overlay
  • core/spaces.py
    • HoloMap
    • DynamicMap

@philippjfr philippjfr added API WIP labels Nov 3, 2018

@philippjfr philippjfr force-pushed the deprecate_methods branch from 0ce6923 to 7ee1f52 Nov 22, 2018

@philippjfr philippjfr changed the title Deprecation of table and mapping methods Deprecation of data conversion methods Nov 22, 2018

@philippjfr philippjfr changed the title Deprecation of data conversion methods Deprecation and consistency of data conversion methods Nov 22, 2018

@philippjfr philippjfr removed the WIP label Nov 22, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Nov 22, 2018

So since .table is still used a fair bit I've removed the deprecation warnings for now and added a config.future_deprecations flag which users can set to see if their code uses deprecated methods which we will start warning about in a subsequent release.

@philippjfr philippjfr changed the title Deprecation and consistency of data conversion methods Deprecation and consistency of core methods Nov 22, 2018

@philippjfr philippjfr changed the title Deprecation and consistency of core methods Deprecation, consistency and docstrings for core methods Nov 22, 2018

@philippjfr philippjfr force-pushed the deprecate_methods branch 3 times, most recently from 8f5f0ac to d6ff0ff Nov 22, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Nov 23, 2018

@jlstevens Ready to review.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Nov 23, 2018

All the suggested deprecations you describe seem very reasonable though (as I mention in #2322) I am not very enthusiastic about these giant docstrings. Are there any tools to validate this format? Check the actual signature matches what is declared in the docstring at least?

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Nov 23, 2018

Ok, I've found myself an emacs code folding solution I can use which makes me happier with these docstrings (at least from a developer perspective). I'll just fold away these docstrings in the files that have them if they stop me seeing the code.

As for Google versus NumPy style, here are my thoughts:

  1. I think I prefer google style a bit as it is more compact.
  2. I also think that if we pick a standard we mustn't change it at all, e.g we shouldn't rename 'parameters' to 'arguments' in the NumPy format. The google format calls them 'Args' so no issue there...
  3. I definitely dislike how the NumPy style forces you to name your return values.
  4. The reason to pick the NumPy style is that both NumPy/Pandas uses it. What big projects use the google style? Will the google style be confusing to people used to the other style?

I personally doubt anyone would find the google style confusing even if they haven't seen it before. This means that on balance, I vote for the google style.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Nov 23, 2018

I'd be happy to switch to Google style, it's what bokeh uses and it is indeed a bit more compact. That said it's an even larger job to convert to that style because the first line with the quotes is expected to provide a summary, something we have never done, e.g.:

    """Fetches rows from a Bigtable.

    Retrieves rows pertaining to the given keys from the Table instance
    represented by big_table.  Silly things may happen if
    other_silly_variable is not None.

    Args:
        big_table: An open Bigtable Table instance.
        keys: A sequence of strings representing the key of each table row
            to fetch.
        other_silly_variable: Another optional variable, that has a much
            longer name than the other args, and which does nothing.

    Returns:
        A dict mapping keys to the corresponding table row data
        fetched. Each row is represented as a tuple of strings. For
        example:

        {'Serak': ('Rigel VII', 'Preparer'),
         'Zim': ('Irk', 'Invader'),
         'Lrrr': ('Omicron Persei 8', 'Emperor')}

        If a key from the keys argument is missing from the dictionary,
        then that row was not found in the table.

    Raises:
        IOError: An error occurred accessing the bigtable.Table object.
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Nov 23, 2018

I think the fact that bokeh uses it makes it more compelling and the short summary line is a good idea anyway imho.

Show resolved Hide resolved holoviews/core/layout.py Outdated
Show resolved Hide resolved holoviews/core/spaces.py Outdated
@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Nov 25, 2018

I'm also going to tackle element docstrings and class hierarchies in this PR, they are all hopelessly outdated.

@philippjfr philippjfr force-pushed the deprecate_methods branch 4 times, most recently from b6349aa to 5cb4c1c Nov 25, 2018

@philippjfr philippjfr force-pushed the deprecate_methods branch from 5cb4c1c to b0af7a3 Nov 27, 2018

Show resolved Hide resolved holoviews/core/overlay.py Outdated
Philipp Rudiger Philipp Rudiger
Show resolved Hide resolved holoviews/core/overlay.py Outdated
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Nov 27, 2018

I am happy with these future deprecations and I do prefer the google style docstrings. I think it will be appreciated by users! I made one comment/suggestion, otherwise I am happy to merge.

Update holoviews/core/overlay.py
Co-Authored-By: philippjfr <philippjfr@continuum.io>
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Nov 27, 2018

The last commit is a tiny change to a docstring and the previous build passed. Merging.

@jlstevens jlstevens merged commit 1347f91 into master Nov 27, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Dec 4, 2018

@jbednar @philippjfr One thing to consider about the google style docstrings is what happens when we want to enable tab completion in IPython.

Unfortunately, IPython only scans the first non-empty line which means you need to insert the signature on the initial line with the short description. At least IPython does seem to scan the whole line so we could append the signature after the short description.

I'm not sure whether tools that use docstrings (e.g sphinx) will pick up on dynamically mutated docstrings (one option is to try to only update docstrings in an ipython context, e.g see the suggestion in pyviz/param#305). One thing that we cannot really fix is the mess a user would see calling help in IPython (I guess hv.help and param.help could have some conditional logic to strip the signature back out though?).

@philippjfr philippjfr deleted the deprecate_methods branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.