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

Override Curve's count aggregator default to self_intersect=False #6030

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

droumis
Copy link
Member

@droumis droumis commented Dec 13, 2023

fixes #5975

This PR disables the self-intersect option for a Datashader count aggregator, which controls whether pixel-crossings of the same line contribute toward a pixel's count.

Why? In some cases, showing self-intersection counts (showing where a line crosses through the same pixel multiple times) is preferred, as it provides information about how the signal varies even when the variations involved are smaller than the pixels on the screen. However, in order to be more consistent with how a non-Datashader curve would appear, it was decided that a more reasonable default behavior would be to disable self-intersect for individual (non-categorical) curves.

As this PR is currently composed, not providing any aggregator argument for hvPlot will adopt the new default. One issue is that setting a count aggregator without any kwargs will revert to adopting Datashader's default (self_intersect=True), is this acceptable?

Code
import datashader as ds
import pandas as pd
import holoviews as hv
hv.extension('bokeh')
import hvplot.pandas

df = pd.read_parquet("./tseriesviz.parq")
df0 = df[df.sensor=='0']
df

width=600
colorbar=False
(df0.hvplot.line('time', 'value', rasterize=True, title='default [count]', line_width=1).opts(width=width, colorbar=colorbar) +\
df0.hvplot.line('time', 'value', rasterize=True, title='Set ds.count()', line_width=1, aggregator=ds.count()).opts(width=width, colorbar=colorbar) +\
 df0.hvplot.line('time', 'value', rasterize=True, title='Set ds.count(self-intersect=False)', line_width=1, aggregator=ds.count(self_intersect=False)).opts(width=width, colorbar=colorbar) +\
df0.hvplot.line('time', 'value', rasterize=True, title='Set ds.count(self-intersect=True)', line_width=1, aggregator=ds.count(self_intersect=True)).opts(width=width, colorbar=colorbar) +\
df.hvplot.line('time', 'value', rasterize=True, by='sensor', title='default [count_cat]', line_width=1).opts(width=width, colorbar=colorbar)).cols(2)

image

@droumis droumis requested a review from hoxbro December 13, 2023 19:03
@droumis droumis added the type: enhancement Minor feature or improvement to an existing feature label Dec 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e23c3e9) 88.62% compared to head (1043e13) 88.62%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6030   +/-   ##
=======================================
  Coverage   88.62%   88.62%           
=======================================
  Files         315      315           
  Lines       65636    65638    +2     
=======================================
+ Hits        58168    58170    +2     
  Misses       7468     7468           
Flag Coverage Δ
ui-tests 23.27% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@philippjfr
Copy link
Member

As this PR is currently composed, not providing any aggregator argument for hvPlot will adopt the new default. One issue is that setting a count aggregator without any kwargs will revert to adopting Datashader's default (self_intersect=True), is this acceptable?

Yes, I think so. It's pretty rare that you'd provide a count aggregator since it's the default.

@hoxbro hoxbro enabled auto-merge (squash) December 14, 2023 08:29
@hoxbro hoxbro disabled auto-merge December 14, 2023 15:24
@hoxbro hoxbro enabled auto-merge (squash) December 14, 2023 15:24
@hoxbro
Copy link
Member

hoxbro commented Dec 14, 2023

Thank you for the PR 💯

The failing test is unrelated.

@hoxbro hoxbro disabled auto-merge December 14, 2023 16:32
@hoxbro hoxbro merged commit b8dd3d0 into main Dec 14, 2023
10 of 12 checks passed
@hoxbro hoxbro deleted the aggcurvedefault branch December 14, 2023 16:32
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Datashader count aggregator self-intersect by default for curves
4 participants