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

Improve docs #32

Merged
merged 12 commits into from Aug 9, 2019

Conversation

@hdsingh
Copy link
Member

commented Aug 3, 2019

I will improve documentation in this PR.

xrviz/__init__.py Show resolved Hide resolved
@hdsingh

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

I have also added sphinx_rtd_theme (that is being used by intake docs).

@hdsingh

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

@martindurant The API doc looks good to me now. Please have a look.

Copy link
Member

left a comment

I have got to the end of "dashboard.py" so far. In general, the API docs could do with more precision and concision.

  • All docstrings should start with a single line and then a blank line
  • Leading words such as "this method" or "provides access to" are to be avoided
  • Tautologies should be removed (e.g., "__init__: initializes this thing")
  • Requires consistency in terms. We have SigSlot instances, which represent and contain various types of panel.Pane
Parameters
----------
data: `xarray` instance: `DataSet` or `DataArray`
datset is used to initialize.
data: `xr.core.dataarray.DataWithCoords`

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

Prefer previous, simple description, xarray.DataSet or xarray.DataArray
You can then specify in the description what that means. I don't expect users to know what DataWithCoords is.

data: `xarray` instance: `DataSet` or `DataArray`
datset is used to initialize.
data: `xr.core.dataarray.DataWithCoords`
Data is required for initialization.

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

This is obvious from the signature, and that you haven't said (optional) or given a default. Instead, we want to say what the role of this input is.

describer: Provides access to `Describe` sub-section.
fields: Provides access to `Fields` sub-section.
kwargs: Provides access to kwargs selected in different subsections.
1. ``panel``: Displays all the panes arraged in form of tabs.

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

A panel.Tabs instance containing the user input panes of the interface.

fields: Provides access to `Fields` sub-section.
kwargs: Provides access to kwargs selected in different subsections.
1. ``panel``: Displays all the panes arraged in form of tabs.
2. ``displayer``: Provides access to `Display` sub-section of `Variables` pane.

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

This is not a useful description.

"A SigSlot instance which shows..."

5. ``fields``: Provides access to `Axes` pane.
6. ``style``: Provides access to `Style` pane.
7. ``projection``: Provides access to `Projection` pane (if present).
8. ``kwargs``: Provides access to values selected in different panes.

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

None of these "provides access" statements are very useful. An attribute always provides access to something. Be very specific, as a scientist would. This last one, for instance:
"a dictionary gathered from the widgets of the input Panes, of a form which can be passed to the plotting function as kwargs"

This method creates a plot according to the values selected in the
widgets. It handles the following two cases:
1. Both `x`, `y` are present in selected variable's coordinates.
2. One or both of `x`, `y` are NOT present in selected variable's coordinates.

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

Please elaborate

@@ -240,7 +248,8 @@ def create_plot(self, *args):

def create_indexed_graph(self, *args):
"""
Creates graph for selected indexes in selectors or players.
Creates a graph for the dimensions selected in `x` and `y`. This

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

What is a graph, opposed to a plot?

This comment has been minimized.

Copy link
@hdsingh

hdsingh Aug 5, 2019

Author Member

I don't think there is any difference. I will replace create_plot with create_graph to have similar function names.

@@ -308,7 +317,10 @@ def create_indexed_graph(self, *args):

def create_taps_graph(self, x, y, clear=False):
"""
To mark taps
This method return a graph to record the taps, when

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

"Create an output layer in the graph which responds to taps from the user

Whenever the user taps (or clicks) the graph, a glyph will be overlaid, and a series extracted at that point"

@@ -330,14 +342,24 @@ def create_taps_graph(self, x, y, clear=False):

def create_series_graph(self, x, y, color, clear=False):
"""
Create series graph
This method extracts a series at the point where user has tapped

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

Extract a series at a given point, and plot it

@@ -457,7 +478,8 @@ def set_coords(self, *args):

def check_is_plottable(self, var):
"""
If a variable is 1-d, disable plot_button for it.
Check if a data variable can be plotted or not. If a variable is 1-d,

This comment has been minimized.

Copy link
@martindurant

martindurant Aug 4, 2019

Member

Remove "or not"

@martindurant

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

If you haven't already, please include "numpydoc" in the extensions listed in conf.py and add it in docs/environment.yml

@hdsingh hdsingh referenced this pull request Aug 5, 2019
@hdsingh

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@martindurant Thanks a lot for such detailed reviews! These are really helpful for me to know more and understand the way docs should be written.

I have made changes according to the review. Please have a look.

hdsingh and others added 3 commits Aug 5, 2019
@martindurant

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

I went through a lot of the docstrings trying to make the text more concise. In the meantime, I may have applied some style guidelines to the code... Please review what I did in the last commit, and apply similar patterns elsewhere.

I still have reservations about exactly what happens when coordinate widgets are updated, I'll chase that up after this PR. I am finding that the xrviz interface within the intake interface is amazingly slow to set widget values, while dfviz is not, and it is essential to get to the bottom of why that might be.

@hdsingh

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

@martindurant I made similar changes and reformat code where required. Documentation looks much better now.
According to me work here is complete. Please let me know if something else needs to be changed in order to merge this PR.

@martindurant

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I think we may still want to circle around to this again, but I'll merge it now, as the PR has become rather long.

@martindurant martindurant merged commit 9eeadb3 into intake:master Aug 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.