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

Date axis item #1154

Merged
merged 58 commits into from Apr 27, 2020
Merged

Date axis item #1154

merged 58 commits into from Apr 27, 2020

Conversation

axil
Copy link
Contributor

@axil axil commented Apr 7, 2020

This is an improvement over PR #74 (see discussion there).

@FilipeMaia
Copy link

Great job! I hope it gets merged...

@2xB
Copy link
Contributor

2xB commented Apr 8, 2020

Thank you! This makes the custom DateAxis in customPlot.py obsolete, therefore I'd replace that with the new DateAxisItem. I've implemented that and sent a PR to @axil's fork.

This leaves the question open whether the examples should contain some kind of custom axis demonstration. Since I do not have a creative idea there, and the old custom axis example was probably anyways only given to provide a DateAxis implementation, I'd for now just place the DateAxisItem there. As soon as there is need for another custom axis item, that can then be added in the way it is needed.

@axil
Copy link
Contributor Author

axil commented Apr 8, 2020

@2xB I've fixed a bug in attachToPlotItem (it said that slot (3,1) was already taken), added two more examples (DateAxisItem itself and attachToPlotItem method to facilitate adding time axis to PlotWidget created with QtDesigner), merged your changes to the customPlot example and modified its description.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 9, 2020

@2xB this ready to merge? @axil thanks for the PR!

@axil
Copy link
Contributor Author

axil commented Apr 9, 2020

@2xB The new two examples do not follow the pattern in the other examples (only two lines behind the if __name__ == "__main__"). Is it important? Should I correct that?

@2xB
Copy link
Contributor

2xB commented Apr 9, 2020

@axil If one could copy-paste the example into an interactive console without breaking it, that would be great. That is why the other examples are written like that. Also, while you are doing that, you could add the header that the other examples have:

"""
Demonstrates [...].

"""

import initExample ## Add path to library (just for examples; you do not need this)

That makes it possible to execute the example from the example folder without installing pyqtgraph.

As this turned into a collection of things to know for examples anyways, here is one more thing: As soon as an example contains much information or is visually interesting, one should add it to the examples library in pyqtgraph/examples/utils.py , so it is also visible in the example browser ( http://www.pyqtgraph.org/documentation/introduction.html#examples ).

@2xB
Copy link
Contributor

2xB commented Apr 9, 2020

Also, I'd like to suggest one more patch that I will probably finish later today, moving the functionality of attachToPlotItem to PlotItem, probably refactoring some code from PlotItem.__init__ into a seperate function, for the sake of more reusability and less maintained code.

@2xB
Copy link
Contributor

2xB commented Apr 9, 2020

@axil The final thing that we would then need to do is to add documentation to the functions that users might use, that can be compiled into the API reference ( http://www.pyqtgraph.org/documentation/apireference.html ). For that we need to write down descriptions and all function parameters in a format similar to the descriptions given in e.g. pyqtgraph/graphicsItems/PlotItem/PlotItem.py. I don't know who of us wants to do that - probably you know more about the functions?

So I'd suggest you go along and work on the examples, I work on my patch, and you say how you'd like to approach documentation, and that's it!

@2xB
Copy link
Contributor

2xB commented Apr 9, 2020

I've just seen that the changes needed for the documentation are very small, I will do them

@axil
Copy link
Contributor Author

axil commented Apr 9, 2020

@2xB Yes, attachToPlotItem is really hacky (both code and the name: not terribly intuitive would I attach to plot item if I create a plot widget), I only included it because it allows to use QtDesigner without subclassing PlotWidget.

The notation

pg.PlotWidget(axisItems = {'bottom': pg.DateAxisItem()}) 

still looks a bit heavyweight for a task as natural as making x axis accept timestamps. Would it be datetime instances it could be determined through typecheck with no args to PlotWidget at all (the way it is done in matplotlib), but unix timestamps are more memory-friendly than datetimes, so it's better to keep timestamps, yet something more concise could be more appropriate. Consider the bokeh library syntax:

p = figure(x_axis_type='datetime')

Yet another alternative is to have a dedicated widget, say, DatetimePlotWidget (cf plot_date in matplotlib, but plot_date doesn't handle time at all, only dates).

Yes, I'd prefer that you updated the documentation. I've added the description to the examples and included the first of them to the utils.py.

@2xB
Copy link
Contributor

2xB commented Apr 9, 2020

Great, thank you! Having a more natural way to set the axis types sounds good. I'd suggest not to do that before you reviewed my upcoming patch though, since that also changes part of the PlotItem __init__ function.

I've also seen that currently a user can crash the application just by zooming too far out. That is bad for practically every type of software based on pyqtgraph. My thought was just to automatically set min and max limits on the DateAxisItem. That is in theory done very easily:

class DateAxisItem(AxisItem):
    [...]
    def linkToView(self, view):
        super(DateAxisItem, self).linkToView(view)
        
        # Set default limits
        _min = ...
        _max = ...
        
        if self.orientation in ['right', 'left']:
            view.setLimits(yMin=_min, yMax=_max)
        else:
            view.setLimits(xMin=_min, xMax=_max)

Practically, I have no idea how to get values for minimum and maximum in this operating system dependent situation.

In the meantime, I am currently implementing a wrapper for utcfromtimestamp containing a try/except. But if there is a solution to the above concept, that'd be great!
Is there a way to get these minimum and maximum values?

@2xB
Copy link
Contributor

2xB commented Apr 9, 2020

There is more to it than I thought: At least on my Linux machine utcfromtimestamp can output dates with years large enough that can't be handled by datetime. And if I wrap that with try/except, when zooming too far out, the axis scaling just stops working. Because of time constraints (pun not intended), I will therefore leave this issue for someone else to fix.

@axil
Copy link
Contributor Author

axil commented Apr 24, 2020

@j9ac9k done

@DanielRudnicki
Copy link

DanielRudnicki commented Apr 24, 2020

Hey, thanks a lot for that feature, it was awaited for a long time and finally there it is:)
Do you know yet when the merge may take place?

@axil
Copy link
Contributor Author

axil commented Apr 26, 2020

@j9ac9k In my opinion this PR is mature enough to get merged to devel branch.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 26, 2020

Hi, @axil ...I asked @ixjlyons and @campagnola to take a look at this PR as well 2 days ago, I wanted to give them at least to the end of the weekend, if I don't hear anything from them I'll merge a day from now.

@axil
Copy link
Contributor Author

axil commented Apr 26, 2020

@j9ac9k Sure, take your time. Just thought everybody has forgotten about it.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 26, 2020

Given the number of outstanding PRs, don't know why you would think that 🤣

Copy link
Member

@ixjlyons ixjlyons left a comment

Choose a reason for hiding this comment

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

I didn't do a very thorough review but this looks good to me. It's a nice feature and the examples seems to work pretty well for me. One other thing I noticed is it would be good to make a doc/source/graphicsItems/dateaxisitem.rst similar to the others in the directory and then add an entry for it in graphicsItems/index.rst. Other than that, looks good to me.


view.sigResized.connect(self.linkedViewChanged)

def unlinkFromView(self):
Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to point out #917 implements a similar feature

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! Since having a seperate method to unlink is the type of implementation you suggested over there, I'd suggest to just borrow the unit tests from over there. If that is agreed on, I'd do that, since unlinkFromView was from me.

from .AxisItem import AxisItem
from ..pgcollections import OrderedDict

__all__ = ['DateAxisItem', 'ZoomLevel']
Copy link
Member

Choose a reason for hiding this comment

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

Should TickSpec be added here as well since ZoomLevel takes a list of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather remove 'ZoomLevel' ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of which: We advertise overriding DateAxisItem.zoomLevels, although the entries of that dict, such as YEAR_MONTH_ZOOM_LEVEL, are not available through __all__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@2xB Does overriding zoomLevels have any practical purpose except for the locale representation of dates (for example someone might like "04.12" for the 4th of December instead of "4 Mon")? Also the names of the levels has become somewhat misleading: "HOUR_MINUTE_ZOOM_LEVEL" level stands for "day minute" actually and "HMS_ZOOM_LEVEL" stands for "second" in fact.

Copy link
Contributor

Choose a reason for hiding this comment

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

The concept of a user-overridable zoomLevels dict actually comes from @3rdcycle in the PR #74 . I think the idea was that at that point, the zoom levels were not as precise as they are now, so there would be a good cause for the user to play around with the zoomLevels dict to make spacing better. Since we are now even DPI-aware, I would agree to you that there is probably no practical use to that and suggest to just remove this from the docstrings.

@axil
Copy link
Contributor Author

axil commented Apr 27, 2020

Unicode issue:
image
I've changed the locale to ru_RU:

import locale
locale.setlocale(locale.LC_TIME, "ru_RU")

I would expect something like "Янв", "Фев", ..., "Дек" in labels.

An easier way to reproduce this is to use any unicode character in the timestamp patterns, eg:

TickSpec(YEAR_SPACING, makeYStepper(1), '%Y€', autoSkip=[1, 5, 10, 25]),

Adding "€" makes ticks disappear at this zoom level.

@axil
Copy link
Contributor Author

axil commented Apr 27, 2020

Ok, setting LC_ALL instead of LC_TIME fixes it. And not accepting '€' is the bug of strftime, not of pyqtgraph. So this one is resolved.

@j9ac9k
Copy link
Member

j9ac9k commented Apr 27, 2020

@axil Thanks for this PR, @2xB thanks for testing it along the way and thanks @ixjlyons for doing another passthrough. This looks good to me, merging!

@TheDigital1
Copy link

TheDigital1 commented Aug 11, 2020

Can someone provide an example, or better yet update the example code, to show how to adjust the axis tick scale? E.g. my use case I want to plot data that is is over a day, so I would like a tick for every hour.
Thanks!

@2xB
Copy link
Contributor

2xB commented Aug 11, 2020

@TheDigital1 A key feature of DateAxisItem is that it automatically adjusts the amount of ticks shown based on how far one has zoomed in (PyQtGraph is an interactive plotting library after all). Do I get it right that you want to overwrite this behavior manually to always show a fixed amount of ticks no matter how far the user zoomed in or out?
Btw. I would generally suggest you to open a new issue instead of posting on a merged Pull Request to assure that your question is actually seen by active people.

@TheDigital1
Copy link

@2xB Makes sense default functionality would be to automatically scale as you said for interactive reasons. However it just so happens in this one particular use case of mine I am using the plot in a non interactive way, and I would like the plot to look correct on the first initialization. The API reference talks about a "setZoomLevelForDensity(density)" function however I am not sure how to use this.

Some more details on my use case:
For example the data I am plotting is 3 minute interval data over 12 hours:

Index Time (US/Pacific) Total Instantaneous Real Power (kW)
0 2020-08-06 00:00:00 18
1 2020-08-06 00:03:00 27
2 2020-08-06 00:06:00 12
.. ... ...
238 2020-08-06 11:54:00 58
239 2020-08-06 11:57:00 89
240 2020-08-06 12:00:00 15

Upon initialization I would like the plot to show all data in view with grid lines at every hour.

Thanks, I wasn't sure if making a new issue made sense as I figured it was just my lack of understanding of how it works and not really an issue. If you think this warrants a new issue let me know and I'll start one.

@2xB
Copy link
Contributor

2xB commented Aug 11, 2020

There exists a "question" label that is put on issues that are not bugs in PyQtGraph, so don't worry there.

Some warning beforehand: The algorithm that you are going to deactivate is sensitive to the DPI of the monitor where the plot is displayed on and the size of the plot. So even if you always show the same plot makes you the responsible person for asserting that nothing overlaps. Also, if it is possible to draw ticks without overlapping labels for every hour, the algorithm will automatically do that for you. If it does not, probably the x axis will look relatively crowded.

Having said that, here is an example for you (a modified version of examples/customPlot.py):

# -*- coding: utf-8 -*-
import pyqtgraph as pg
import numpy as np

app = pg.mkQApp()

class CustomDateAxisItem(pg.DateAxisItem):
    def tickValues(self, minVal, maxVal, size):
        # Always use HOUR_MINUTE_ZOOM_LEVEL
        self.zoomLevel = pg.graphicsItems.DateAxisItem.DAY_HOUR_ZOOM_LEVEL

        # Generate values
        first_hour = np.ceil(minVal/3600)*3600
        ticks = np.arange(first_hour, maxVal, 3600)
        spacing = pg.graphicsItems.DateAxisItem.HOUR_SPACING
                # Has to be a spacing used for the self.zoomLevel we use
                # As a zoomLevel consists of two TickSpecs, the spacing indicates,
                # which of these two specs should be used to generate the ticks
        
        return [ (spacing, ticks) ]

vb = pg.ViewBox()

axis = CustomDateAxisItem(orientation='bottom')

pw = pg.PlotWidget(viewBox=vb, axisItems={'bottom': axis})

x = np.linspace(0.1,1.1,20) * (3600*12)
y = 1/x

pw.plot(x=x, y=y, symbol='o')
pw.show()

## Start Qt event loop unless running in interactive mode or using pyside.
if __name__ == '__main__':
    app.exec_()

@2xB
Copy link
Contributor

2xB commented Aug 11, 2020

@TheDigital1 .

@TheDigital1
Copy link

@2xB Just ran your example code. Looks good! I'll work to integrate it into my code and see if I get the results I'm looking for.
Thanks so much!

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.

None yet

8 participants