Skip to content

Fix doubling labels in DateAxisItem#2413

Merged
j9ac9k merged 5 commits intopyqtgraph:masterfrom
bbc131:origin/dateaxisitem_doubles
Sep 14, 2022
Merged

Fix doubling labels in DateAxisItem#2413
j9ac9k merged 5 commits intopyqtgraph:masterfrom
bbc131:origin/dateaxisitem_doubles

Conversation

@bbc131
Copy link
Copy Markdown
Contributor

@bbc131 bbc131 commented Sep 12, 2022

In short

  • Due to floating point errors, comparison for equality sometimes fails to detect doubling values. Use approximative comparison instead. Code is copied from AxisItem.tickValues().

In detail

In some situations DateAxisItem draws two different tick labels (of different levels) for the same value. See the following screenshot, where "60.0" and "02:01:00" are drawn. "60.0" should be skipped and only "02:01:00" should be drawn.
This can be reproduced, using the DateAxisItem example, simply zoom in like in the screenshot. There, the visible range is something between 0.01s and 0.004s. It seems to happen at each full minute. Note, that the occurence depends also on the minimum axis value, meaning it may be necessary to pan the axis a bit to the left or right always keeping the full minute within the visible range.

grafik

* Due to floating point errors, comparison for equality
  sometimes fails to detect doubling values.
  Use approximative comparison instead.
  Code is copied from AxisItem.tickValues().
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Sep 12, 2022

Hi @bbc131

Thanks so much for your work on DateAxisItem, it's one of the relatively newer parts of the library, and clearly has some rough corners. I appreciate your work on improving it.

Slight nitpick suggestion, no need to cast to list the filter object; extending a list with a filter object will address that conversion for you.

An alternative would be to have allTicks be a numpy array (like allValues in AxisItem), and concatenate the array instead of extending it.

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Sep 13, 2022

A numpy broadcasted version:

ticks = np.array(ticks)
close = np.any(np.isclose(allTicks, ticks[:, np.newaxis], rtol=0, atol=minSpc*0.01), axis=-1)
ticks = ticks[~close].tolist()

* Now, it's more like in AxisItem.tickValues()
* Use `numpy.isclose()` instead of `filter()` with lambda
@bbc131
Copy link
Copy Markdown
Contributor Author

bbc131 commented Sep 13, 2022

Hi @j9ac9k and @pijyoi,
Thanks for your fast response and feedback.
I like the suggestion of @pijyoi, and implemented it, also for AxisItem. I like it, if the code of both classes differs as little as possible.

valueSpecs.append((spec.spacing, tick_list))
# we assume here that if the difference between a tick value and a previously seen tick value
# is less than min-spacing/100, then they are 'equal' and we can ignore the new tick.
ticks = np.array(ticks)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose this line is redundant since ticks is already an ndarray, something that wasn't obvious to me at the time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, I updated it.

* `ticks` is already a `numpy.ndarray`
)
ticks = ticks[~close]
allTicks = np.concatenate([allTicks, ticks])
valueSpecs.append((spec.spacing, ticks))
Copy link
Copy Markdown
Contributor

@pijyoi pijyoi Sep 14, 2022

Choose a reason for hiding this comment

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

To respect the description in the method docstring, ticks would need to be converted to a Python list here.
i.e.

valueSpecs.append((spec.spacing, ticks.tolist()))

)
values = values[~close]
allValues = np.concatenate([allValues, values])
ticks.append((spacing/self.scale, values))
Copy link
Copy Markdown
Contributor

@pijyoi pijyoi Sep 14, 2022

Choose a reason for hiding this comment

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

To respect the description in the method docstring, values would need to be converted to a Python list here.
i.e.

ticks.append((spacing/self.scale, values.tolist())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for being so attentive. I see, you had this correct already in your very first comment, but I removed it silently ;)

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Sep 14, 2022

LGTM, thanks @bbc131 for the PR thanks @pijyoi for the review/feedback!

@j9ac9k j9ac9k merged commit d0b9b7c into pyqtgraph:master Sep 14, 2022
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.

3 participants