ENH/DEP: use a more robust, external implementation of the Line Integral Convolution (LIC) algorithm #5131
Conversation
neutrinoceros
added
bug
viz: 2D
proposal
| lim=(0.5, 0.6), | ||
| cmap="binary", | ||
| alpha=0.8, | ||
| alpha=0.5, |
There was a problem hiding this comment.
I found that a lower default alpha (opacity) made more sense with the correct implementation where "ink" is more uniformly distributed across the image so it's easier to hide the background completely if you're not careful.
| mask = ~(np.isfinite(pixX) & np.isfinite(pixY)) | ||
| lic_data[mask] = np.nan | ||
| lic_data_clip[mask] = np.nan |
There was a problem hiding this comment.
This part was initially added to fix #3738
rLIC natively handles NaNs in the input vector fields: streamlines are simply terminated as soon as a NaN is encountered, so yt doesn't need to clean after itself anymore.
| @@ -3238,10 +3238,10 @@ def __init__( | |||
| field_x, | |||
| field_y, | |||
| texture=None, | |||
| kernellen=50.0, | |||
| kernellen=50, | |||
There was a problem hiding this comment.
a float never made sense here: kernellen represents the length of a 1D array. I'm just fixing this in passing.
|
(I'm putting this on the 4.5.0 milestone, mostly as to mean I don't want it to be backported, but it shouldn't block any particular release) |
|
whoa. |
| if self.texture is None: | ||
| prng = np.random.RandomState(0x4D3D3D3) | ||
| self.texture = prng.random_sample((nx, ny)) | ||
| elif self.texture.shape != (nx, ny): |
There was a problem hiding this comment.
I removed this check because rlic.convolve already runs sanity checks over its inputs, including this one.
neutrinoceros
changed the title
|
@olebole, do you have any objection to, or questions about adding this dependency to yt ? Would you expect it could be a burden to package rlic for debian ? |
|
@neutrinoceros I will not be able to package it for the upcoming Debian "Trixie" release which will come summer/autumn of this year, but for later this should be possible. |
|
Happy to go down this road with you, it is also my first experience mixing rust and Python, so I'm genuinely curious to find how easy or difficult the overall process turns out ! |
|
Let's see how it plays out over at conda-forge first conda-forge/staged-recipes#29385 |
|
conda-forge is done. |
…Integral Convolution (LIC) algorithm
|
power cycling this to refresh CI |
|
Moving to rLIC should soon unblock the missing feature for LIC: compat with periodic datasets neutrinoceros/rlic#196 |
PR Summary
I've been spending some time rewriting the Line Integral Convolution (LIC) algorithm in Rust and published it as a new, minimal, Python package: rLIC
Because I wanted to see how it compared with yt's builtin Cython extension, in terms of performance, I ran a couple tests, and found that
rLICwas not only much faster1 (more than x2):this benchmark shows the execution time of convolving a 512 x 512 image with various kernel sizes
... it also seems to be more correct. See this very simple test case, where yt's implementation appears to be completely broken:
This is the one example from our docs, as of yt 4.4.0
and here it is with this branch
so, instead of fixing yt's implementation, I would like to propose yt adopts
rLICas a dependency.Since yt's implementation doesn't appear to be correct, it is no surprise that
rLICisn't a drop-in replacement: new results are not identical. Bug-for-bug backward compatibility being excluded, I also made a couple adjustments, like replacing the legacy prng API that's discouraged in numpy.Important notes:
rLICwheels areabi3, which means that the package is already future compatible with Python 3.14+ and will remain so until CPython breaks abi3 and creates an abi4. If this happens, I will publish new wheels. The point being that depending onrLICis not expected to cause any disturbance when adding support for newer Python versions in ytrLICis not yet distributed through conda-forge, which I'm aware is a requirement for any dependency of another package that wants to be shipped there. I don't mind putting the package on conda-forge too, pending acceptance of this proposalupdate: nevermind I'm going for it anyway Add rlic conda-forge/staged-recipes#29385
update 2: it's up on conda-forge
PR Checklist
Footnotes
if you're wondering, no, it's not just because my version is in Rust. I did spend time optimizing the implementation, in particular I avoided wasteful speculative executions which are extremely common with the naive implementation of this algorithm. ↩