Documentation text review for PlotDataItem#3057
Conversation
| After a search has been performed, typically during a call to | ||
| :func:`dataRect() <pyqtgraph.PlotDataset.dataRect>`, ``dataset.containsNonfinite`` | ||
| is ``True`` if any coordinate values are non-finite (e.g. NaN or inf) or ``False`` | ||
| is ``True`` if any coordinate values are non-finite (e.g. ``NaN`` or ``Inf``) or ``False`` |
There was a problem hiding this comment.
I keep going back and forth on this, but should we replace usages of NaN with np.nan and np.inf for Inf?
There was a problem hiding this comment.
The argument for NaN is that
- it is more generic
- data input is not necessarily in numpy format.
When we generate a NaN, it should probably be np.nan, but I think that does not occur here as far as the documentation is concerned.
|
|
||
| The symbols of a scatter point are rendered over the "point" of the data, and the | ||
| curve connects adjacent points. | ||
| PlotDataItem is PyQtGraph's primary way to plot 2D data. It provides a unified |
There was a problem hiding this comment.
First line in the docstring needs to be confined to 1-line per numpydoc:
https://numpydoc.readthedocs.io/en/latest/format.html#short-summary
| :func:`~pyqtgraph.mkColor` | ||
| See for supported color arguments. | ||
| :func:`mkBrush <pyqtgraph.mkBrush>` | ||
| for supported arguments. |
| :func:`mkBrush <pyqtgraph.mkBrush>` | ||
| for supported arguments. | ||
| :func:`mkColor <pyqtgraph.mkColor>` | ||
| for supported color arguments. |
| -------- | ||
| :class:`pyqtgraph.FillBetweenItem` | ||
| See for another :class:`~pyqtgraph.GraphicsItem` that fills in regions. | ||
| for another :class:`~pyqtgraph.GraphicsItem` that fills in regions. |
|
I'll review the content in a bit, but numpydoc has some ...opinions. Link to the style guide, but I know it's a lot of text so it's tough to recognize all the issues. https://numpydoc.readthedocs.io/en/latest/format.html#sections Main ones when I ran numpydoc (I should note, that for numpydoc to run, I'm having to run my own fork, as they haven't merged my PR). Some samples of the errors/issues it returns: Descriptions should be capitalized: First line is a "short summary" which should fit on one line: Here's the whole battery of "errors" it returned (note that not all these can be fixed, the 1st and the 3rd items I could not fix). |
j9ac9k
left a comment
There was a problem hiding this comment.
Just some formatting nit-picks. The content is a net improvement across the board, thanks so much!
| Skip the check for bypassing the checking and compensating | ||
| for ``np.nan`` values. If ``connect='auto'``, this item | ||
| will be overridden. | ||
| If `True`, the special handling of non-finite values such as |
There was a problem hiding this comment.
nit-pick level 1,000.
Should be
``True``
| ---------- | ||
| alpha : float | ||
| Value passed to :meth:`QGraphicsItem.setOpacity`. | ||
|
|
There was a problem hiding this comment.
I don't think we want a newline here?
| return | ||
| self.opts['alphaHint'] = alpha | ||
| self.opts['alphaMode'] = auto | ||
| self.opts['alphaMode'] = auto # 'alphaMode' option is never used or transferred, should be deprecated. |
There was a problem hiding this comment.
when experimenting with removing the usage of alphaMode, it blew up in my face. I didn't have a good grasp of what alphaMode was actually a representation, but removing the argument altogether from the function signature didn't work! Likely that's used elsewhere.
There was a problem hiding this comment.
Thank you, I wrote that first and then forgot to delete it.
My impression is that PlotItem is using this to leave itself notes.
Maybe the real issue here is if we consider this a public function or if it is ONLY intended to be called internally.
| :class:`pyqtgraph.FillBetweenItem` | ||
| See for another :class:`~pyqtgraph.GraphicsItem` that fills in regions. | ||
| This | ||
| :class:`~pyqtgraph.GraphicsItem` creates a filled in region between two |
There was a problem hiding this comment.
probably doesn't need to be a new line here?
There was a problem hiding this comment.
Errr... It does. Without the newline, :class:~pyqtgraph.GraphicsItem doesn't parse correctly, that is, it literally appears as
:class:`~pyqtgraph.GraphicsItem`
in the html. I am not sure if this is a parser bug, or if I did something that left the parser in an invalid state before.
| @@ -945,13 +1002,14 @@ def setSymbolBrush(self, *args, **kwargs): | |||
|
|
|||
| def setSymbolSize(self, size: int): | |||
There was a problem hiding this comment.
oh, this should be updated to be size: int | list[int] ...not your change, but a mistake I made.
There was a problem hiding this comment.
There is some air of uncertainty on whether list includes np.ndarray, or if and when that should be array_like.
I have no suggested answer to that question.
|
The linter comments are very specific and really helpful in targetting the correct style. There is just one issue I wanted to bring up for discussion: In the "See Also" section, the linter demands a full sentence of explanation. Here is an example: These sentences often seem a little awkward and difficult to connect to the linked target. My impression is that it is easier, shorter, and at least equally clear to write in a way that joins the section title, linked target and comment into one sentence: that is "See also \ mkBrush \ for supported arguments." I do like that the linter encourages a consistent style here, which would be harder if we disable the rule that encourages capitalization, i.e. a full sentence after the reference. Do we have any additional opinions on this? |
Not all the linter rules make sense for us, I have no problem disabling it (and think in this case it makes sense to do so). An alternative is we do not use the |
| - ``'finite'`` - creates a break when a non-finite points is | ||
| encountered. | ||
| - :class:`~numpy.ndarray` - it should contain `N` elements that of | ||
| - ``'auto'`` connects all points, but creates a break where a |
There was a problem hiding this comment.
"connects all points, but creates a break..." sounds contradictory.
Looking at the implementation, "auto" semantically means "finite".
In the absence of non-finites, "all" is equivalent to "finite".
Since "auto" only uses "all" when there are no non-finites, it is equivalent to using "finite".
Under the hood, the combination of "all" + skipFiniteCheck=True goes through a different processing path, but that is an implementation detail.
There was a problem hiding this comment.
I know this isn't that relevant to this PR, but the skipFiniteCheck was intended as a band-aid, which may no longer be relevant, in which case I would be fine with removing.
There was a problem hiding this comment.
Thank you for the comments! I'll reconsider the wording to avoid the whiplash going from "connect all" to "creates breaks".
My understanding is that the mode we want to encourage is the one currently called 'auto'. It is the same as 'finite', but if e.g. log mode calculations have already given us the info that there are no non-finites, then the test is skipped for free.
Two alternative proposals to make this nicer and simpler:
(A) If skipFiniteCheck is indeed not needed anymore (is it?), then we should simply stop advertising 'auto' mode and eventually deprecate it.
(B) If the 'auto' behavior is our best default solution, then we should rename 'auto' to 'finite'. This would restore parity between the 'connect' options in PlotCurveItem and PlotDataItem. As you correctly point out, there is no functional difference on the user side. ('auto' would remain as a hidden alias for compatibility.)
There was a problem hiding this comment.
There are 3 possible modes here:
- connect="all" + skipFiniteCheck=True
- purpose: data is known to be a single segment with all data finite
- connect="all" + skipFiniteCheck=False
- purpose: data may have non-finites, but user wants to draw it as a single segment anyway (by effectively eliminating the non-finite values)
- connect="finite" + skipFiniteCheck=False ("finite" implies that checking cannot be skipped)
- purpose: draw runs of finite values as separate line segments
It can be observed that (1) is a special case of (3). (ignoring side-effects like whether one runs more efficiently than the other)
In the current implementation of PlotDataItem, connect="auto" will result in either (1) or (3) only. Hence, rendering-wise, connect="auto" is equivalent to "finite".
One possible reason for the existence of (2) is that things like fill and histogram do not behave well in the presence of non-finites. i.e. (2) exists to sanitize the data for code that can't (and/or won't be updated to) handle non-finites.
There was a problem hiding this comment.
Agreed that there are really only two distinctions.
These should be distinguished by user intention, not implementation details and history.
[I want a single line, ignore bad points that slipped in] <---> [I want interruptions to be visible as interruptions]
These intentions are 'all' and 'finite', respectively.
We should get rid of 'auto', because it indeed renders equivalent to 'finite'.
However, I think the implemention of 'auto' goes in the right direction: It makes better use of available information and avoids duplicating work. (Even if it is not as smart now as it could probably be.)
So proposal (B) is to
- remove what PlotDataItem now considers "finite" mode.
- make sure that the current "auto" codepath respects 'skipFiniteCheck' when there is no known information.
- rename "auto" to "finite"
That would preserve all functionality while removing the confusion of having an extra option that's poorly distinguished.
Is there a more elegant way?
[edit] I think historically the Qt behavior was 'finite'. When that changed and broke all plots with NaNs, the first fix was equivalent to 'all'. In any case, both of these are well established and need to stay.
There was a problem hiding this comment.
I don't understand your second bullet point about respect-(ing) skipFiniteCheck. Did you mean that if no known information, then use the user supplied value of skipFiniteCheck?
Currently, as implemented and documented, "auto" means user supplied skipFiniteCheck is ignored. This makes sense and is the current value-add of "auto" over "finite".
Perhaps skipFiniteCheck in PlotDataItem should be removed? (while retaining it for PlotCurveItem)
It can retroactively be defined that PlotDataItem's skipFiniteCheck argument was only ever a hint that may or may not be respected.
In fact, we could follow ImageItem's model where instead of just indicating whether non-finites are present, we provide (and cache) the locations (indices) of the non-finites to PlotCurveItem. An empty locations would indicate that all data is finite.
In such a model, PlotDataItem doesn't get to say that it doesn't know if the data has any non-finites. It has to find it. After all, any time saved in PlotDataItem just gets kicked down the line to PlotCurveItem.
There was a problem hiding this comment.
You are right, there is clearly an alternative architecture that would make this a lot more logical and accessible.
Having PlotDataItem always check for and locate any non-finite values would fit well with my current understanding of how processing should be split between PlotDataItem and its curve and scatter children.
A specific proposal for that will need its own PR though. For this PR, I have tried to clean up the description for the current state of implementation a little, but that is where I would like to leave it for now.
After the recent formatting update to the PlotDataItem documentation in PR #3039, I have gone through the text itself.
I would like to propose some changes for consistency and readability. Very likely not all changes are good, but I hope some of the better ones can be picked out and moved into the documentation.