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

Correctly formatting DateAxisItem ticks in different timezones #1389

Open
NomAnor opened this issue Oct 1, 2020 · 4 comments
Open

Correctly formatting DateAxisItem ticks in different timezones #1389

NomAnor opened this issue Oct 1, 2020 · 4 comments

Comments

@NomAnor
Copy link

NomAnor commented Oct 1, 2020

Short description

Formatting of of ticks is not correct when the output format should be in a timezone != UTC.

Code to reproduce

from PyQt5 import QtWidgets
import pyqtgraph as pg
from datetime import datetime, timezone, timedelta
import pytz



class DateAxisItem(pg.DateAxisItem):
    def __init__(self, timezone, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.enableAutoSIPrefix(False)
        self.utcOffset = 0
        self.timezone = timezone

    def tickStrings(self, values, scale, spacing):
        formatStrings = []
        for value in values:
            dt = datetime.fromtimestamp(value, tz=self.timezone)
            formatStrings.append("{:%H:%M}".format(dt))
        return formatStrings


europe = pytz.timezone('Europe/Berlin')
nepal = pytz.timezone('Asia/Kathmandu')
timezone = europe

app = QtWidgets.QApplication([])
axis = pg.DateAxisItem(orientation='bottom')
axis.utcOffset = -2*60*60 # this can't replace a timezone specification
pw = pg.PlotWidget(axisItems={ 'bottom': axis, 'top': DateAxisItem(timezone) })
pw.showAxis('top')
pw.showGrid(x=True)

start = datetime(2020, 10, 24, 22, 0, tzinfo=pytz.utc)
data = [ (start + timedelta(minutes=30*i), i) for i in range(10) ]

print("i UTC                   Europe")
for dt, i in data:
    print("{} {:%Y-%m-%d %H:%M:%S}   {:%Y-%m-%d %H:%M:%S}".format(i, dt.astimezone(pytz.utc), dt.astimezone(timezone)))

pw.plot([ d[0].timestamp() for d in data ], [ d[1] for d in data ], symbol='o')
pw.show()
app.exec_()

Expected behavior

On 2020-10-25 03:00 CEST Germany changes back to 2020-10-25 02:00 CET so the Axis
should show two ticks with the same time one hour apart. The first tick represents
2020-10-25 02:00 CEST, the second 2020-10-25 02:00 CET.

Real behavior

grafik

The ticks show 02:00 followed by 03:00

Tested environment(s)

  • PyQtGraph version: 0.11.0
  • Qt Python binding: PyQt5 5.15.1 Qt 5.15.1
  • Python version: 3.8.5
  • NumPy version: 1.19.2
  • Operating system: Arch Linux
  • Installation method: pip with virtualenv

Additional context

Using the utcOffset Parameter can't give the correct behaviour because the offset changes depending on the timepoint.

Shifting the posix timestamp values by some offset is not the correct way to translate to different
timezones. Posix timestamps are not timezone aware, they always represent the seconds after the epoch
1970-01-01 00:00 UTC.

It's easiest to show for DST transitions but the same applies for all timezones because
the timezone offsets can change irregularly.

My example code shows the normal DateAxisItem on the bottom and simple patched one on the top
which shows the correct behaviour.

I've briefly look into how the utcOffset is used in calculating the tick values. I would expect the ticks
to line up with the local time (e.g. a tick every full hour). This is not the case, because
there are timezones with offsets that are not multiples of one hour (e.g. UTC+05:45 used in Nepal).

Maybe I misunderstood the purpose of the utcOffset parameter, the documentation is missing for it.

Resolution

For my current project I have to implement a correct AxisItem so I could create a pull request.
What should be the interface? Can the current DateAxisItem be changed or should a new AxisItem be introduced?

I think something like NewDateAxisItem(timezone=None, zoomLevels=None, **kwargs). Timezone None means the local timezone, same as in datetime. None for zoomLevels means the default zoom levels similiar to the current.
These would be the main customization points without the need to subclass.

The documentation should show an example how to correctly create posix timestamps from datetime objects.

@NomAnor
Copy link
Author

NomAnor commented Oct 2, 2020

Here is my implementation: https://gist.github.com/NomAnor/1eb616c950a4154793bd6c235038ebfb

It adds an additional dependency on datetutil that is kind of needed anyway because of the better timezone implementation.

My implementation:

  • Outputs the timestamps formatted in the given timezone
  • Always aligns on local time
  • Puts ticks at multiples of a given timestamp part instead of having a fixed distance. I think this gives more readable output.
  • Uses a similar logic as the current implementation

In some cases using non-aligning DateTickSpecs for different tick levels results in ticks drawn too close (e.g. MONTH_DAY_ZOOM_LEVEL, day 30 overlapping the following month name on day 1). This could be ignore as a minor problem or
an additional check could be implemented in DateZoomLevel.tickValues().

If I get positive feedback from the developers I will create a pull request.

@j9ac9k
Copy link
Member

j9ac9k commented Oct 22, 2020

hey @NomAnor sorry I didn't follow up w/ this earlier. @2xB was the maintainer that largely drove the effort for DateAxisItem; and he's had a leave of absence, ...we're a bit reluctant to introduce dependencies, but if you are able to structure the use of dateutil such that it's optional; and pyqtgraph won't blow up on you for not having it installed (unless you're doing this kind of interaction w/ the library) we would certainly welcome a PR.

Furthermore, if you add some test code to test this functionality; we'll feel even better about it 😆

I am looking over you gist, and I'm not seeing any major issue with it (well, besides its not py2 compatible, but we can worry about that later); but likely I would experiment with it a bit, which having a PR handy would make it easier for me to do so.

Thanks for creating the issue!

@2xB
Copy link
Contributor

2xB commented Nov 9, 2020

Sorry for answering late, I'm not consistently working on this project. Anyways:

Implementing Daylight Saving Times in DateAxisItem is certainly useful! Therefore I would also suggest to make a PR. For that I have a few hints, since developing on a relatively large collaborative project has some limitations from best practices that one might be used to:

First, pyqtgraph tries to be as backwards compatible as possible. So renaming functions, classes etc. or changing the order of - or removing - function arguments or changing their defaults all should only happen when absolutely needed. This is because ones application should ideally work without further effort also after updating pyqtgraph.

Then, I would try to keep the PR minimal. Don't get me wrong, I really appreciate the overhaul you did! Just the more one changes, the more has to be checked for possible implications. We had quite many comments in the original DateAxisItem PR #1154 leading to 58 commits, quite a few of them fixing edge cases. So with any major overhaul, this effort has to be repeated. As far as I see, e.g. the fix for the edge case of correct label size estimations for high-DPI monitors is lost in your overhaul.

So all in all great work, and depending on whether you can afford this time-wise, I'd suggest to use the knowledge gained from this to make a PR touching less code and being more backwards-compatible. Thank you again for the work done so far!

@NomAnor
Copy link
Author

NomAnor commented Nov 11, 2020

That may make it bit difficult to implement. If I have time, I look into it and create a PR when I have a working prototype.

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

No branches or pull requests

3 participants