holoviz / holoviews

With Holoviews, your data visualizes itself.
https://holoviews.org
BSD 3-Clause "New" or "Revised" License
2.66k stars 396 forks source link

Make Dimension.label source of truth for Dimension identity #6262

Closed philippjfr closed 4 weeks ago

philippjfr commented 1 month ago

Implements https://github.com/holoviz/holoviews/issues/6260

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 80.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 88.52%. Comparing base (9cc87fe) to head (7b0f15f). Report is 9 commits behind head on main.

Files Patch % Lines
holoviews/plotting/mixins.py 61.53% 5 Missing :warning:
holoviews/plotting/mpl/chart.py 69.23% 4 Missing :warning:
holoviews/plotting/bokeh/graphs.py 0.00% 2 Missing :warning:
holoviews/plotting/mpl/graphs.py 0.00% 2 Missing :warning:
holoviews/plotting/util.py 71.42% 2 Missing :warning:
holoviews/plotting/bokeh/path.py 50.00% 1 Missing :warning:
holoviews/plotting/plotly/chart.py 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6262 +/- ## ========================================== + Coverage 88.47% 88.52% +0.04% ========================================== Files 323 323 Lines 67647 67906 +259 ========================================== + Hits 59853 60116 +263 + Misses 7794 7790 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

philippjfr commented 4 weeks ago

Is this an ok (rough) understanding?

Yes.

How thoroughly have you checked the codebase for this find and replace? The code still seems to use name for range lookup.

Clearly not quite thoroughly enough, will do another more exhaustive check now.

Have you checked if hvplot / GeoViews need updating too?

Yes.

philippjfr commented 4 weeks ago

Did miss quite a few. Will scan more thoroughly one more time. Note that this only affects plotting code, so e.g. this isn't affected:

holoviews/holoviews/operation/normalization.py

Line 109 in c8e7f5f

 return {d.name: element.range(d.name, self.data_range) 
philippjfr commented 4 weeks ago

Okay, quite confident now, thoroughly reviewed the plotting code and replaced all remaining cases.

philippjfr commented 4 weeks ago

I think the only way we'll catch any more cases will be if we merge and include the change in the RC release.