-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Use contourpy for contour calculations #5910
Conversation
except ImportError: | ||
raise ImportError("contours operation requires matplotlib.") | ||
extent = element.range(0) + element.range(1)[::-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer used as I don't think it is required. There is explicit code to set the x
and y
arrays and pass them through, so the extent
kwarg would have been ignored.
|
||
offsets = lines[1][0] | ||
if len(offsets) > 2: | ||
# Casting offsets to int64 to avoid possible numpy UFuncOutputCastingError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a concern. It isn't needed for test_operation.py
but is for one of the KDE tests in test_statsoperations.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the type of the KDE test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a clean environment this is now needed to avoid the "UFunc add" error in many tests (test_operation.py
as well as test_statsoperations.py
). The complaining line is in numpy function_base.py
when adding extra values to these uint32
offsets. The values added could in theory be negative (although they are always positive in practice) and hence it doesn't like the unsigned-ness of uint32
. int64
is the next largest signed integer, so it seems a sensible choice.
Codecov Report
@@ Coverage Diff @@
## main #5910 +/- ##
==========================================
- Coverage 88.58% 88.45% -0.13%
==========================================
Files 313 315 +2
Lines 65066 65442 +376
==========================================
+ Hits 57636 57887 +251
- Misses 7430 7555 +125
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 68 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Let us add it to
Ok.
I think it should be safe to convert it to
Looking at the code for
I think assertEqual converts it before comparing it. I would suggest using pandas or numpy testing modules directly.
Wouldn't a conversion of datetime data to int64 solve this? Maybe not perfect, but good enough. We can always add a more complex implementation if there is a demand for it. |
I've added a minimal |
For the conversion of datetimes to numbers, shouldn't it be floats rather than ints to include fractional seconds? For example something like as_floats = some_datetimes.astype('datetime64[s]').astype(np.float64)
back_to_datetimes = as_floats.astype('datetime64[s]') |
Really appreciate this @ianthomas23, I've been meaning to do this ever since you first mentioned contourpy. |
7489db1
to
eb20d49
Compare
The low-level contour operation now works as expected. I have replaced use of the Matplotlib functions More work is needed though. All the unit tests are passing, but if you try out a filled contour plot in Jupyter using one datetime dimension then it fails as there are other places in the code where holoviews/holoviews/plotting/bokeh/util.py Lines 1045 to 1068 in 8ba691a
|
eb20d49
to
b49fa15
Compare
b49fa15
to
662d4fb
Compare
Co-authored-by: Simon Høxbro Hansen <[email protected]>
30d49fd
to
4f9af4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left some minor comments. But overall it looks great.
Thank you for modernizing the contouring code to use state-of-the-art algorithms 👍 |
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. |
This is a WIP to use ContourPy for contour calculations. It was originally intended just to fix #5899, caused by a change within Matplotlib of how contour data is internally stored, but it is easier and better just to use ContourPy directly. This is faster and avoids unnecessary generation and manipulation of extra contour data such as Matplotlib
path
codes.Notes:
levels
kwarg is an integer. This currently uses theMaxNLocator
class. The required functionality could be directly vendored within HoloViews to completely remove the need for Matplotlib, e.g. if using the Bokeh backend.x
,y
, and/orz
arrays. There were no tests of these so have added some. Unfortunately some of these new tests such astest_image_contours_x_datetime
pass regardless of the comparisoncontour
in theassertEqual
. I suspect thatassertEqual
silently ignores datetimes.z
data implies datetimelevels
too. To fully support this we may not be able to rely onMaxNLocator
but might need to also use or implement some of Matplotlib'sDateLocator
functionality.