Add new filter sample_over_line and refactor plot_over_line #525
Conversation
pyvista/core/filters.py
Outdated
| if resolution is None: | ||
| resolution = int(dataset.n_cells) | ||
| if not isinstance(resolution, int) or resolution < 0: | ||
| raise RuntimeError('`resolution` must be a positive integer, not {}'.format(type(resolution))) |
There was a problem hiding this comment.
I'd expect the resolution checking to happen in pyvista.Line.
There was a problem hiding this comment.
Ah good point! It would be good to go ahead and move that check over to thee pyvista.Line function
There was a problem hiding this comment.
I did nothing but refactor the code, so this was original, but it is always good to clean things up. Let me dig deeper, I suppose this is a chance to learn more about pyvista internals.
There was a problem hiding this comment.
Yep, this one is on me when I originally wrote this filter. If you could move that check over to the following function, it would be a help!
There was a problem hiding this comment.
With the current version if I do
pv.Line([0, 0, 0], [0, 0, 1], resolution=10.5)I get a helpful error already from presumably vtk
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/redacted/pyvista/pyvista/utilities/geometric_objects.py", line 348, in Line
src.SetResolution(resolution)
TypeError: SetResolution argument 1: integer argument expected, got floatDo you still prefer a check inside pyvista with RunTimeError?
There was a problem hiding this comment.
I didn't touch this part yet as I'm not sure what you want. My understanding is that ValueError is for arguments of the wrong type, which seems to fit here.
There was a problem hiding this comment.
ah that's a good point - then I would just let that chack from vtk be it
There was a problem hiding this comment.
i.e. no need to even have a check inside pyvista
There was a problem hiding this comment.
ok more subtleties....
pv.Line([0, 0, 0], [0, 0, 1], resolution=0)
pv.Line([0, 0, 0], [0, 0, 1], resolution=-1)All of these lines return a line with two points. I think we should still check the magnitude to be positive in Line, which is in the newest commit I pushed.
There was a problem hiding this comment.
Great idea! I think it would be best to have the filter return a PyVista mesh object rather than two numpy arrays. A lot more can be done with the PolyData line such as creating a tube and plotting it in 3D. Also, this would allow you to sample all arrays in the dataset at once
|
I removed the check for integer resolution, but moved the check for positive resolution (this one isn't checked in vtk) to |
MatthewFlamm
changed the title
|
@MatthewFlamm, I'm happy with this at this point, are you finished? @imsodin, does this look good to you now? |
|
I'm done, unless there are other comments. |
| line = pyvista.Line(pointa, pointb, resolution=resolution) | ||
| sampled = line.sample(dataset) | ||
| # Sample on line | ||
| sampled = DataSetFilters.sample_over_line(dataset, pointa, pointb, resolution) |
There was a problem hiding this comment.
EDIT: Actually this seems to be valid, wouldn't have expected that but on second thoughts it makes sense. Still in my opinion:.
This must be dataset.sample_...
More generally maybe there should be a static analyser on CI to catch such things.
There was a problem hiding this comment.
I've implemented a lot of filters that call other filters this way. I thought it was "safer"
There was a problem hiding this comment.
is this bad practice, @imsodin?
There was a problem hiding this comment.
This whole class is littered with both types examples. What is the preferred approach, and I can use that one?
There was a problem hiding this comment.
I'm happy with it as is, but yes, you're right that the class has both examples all over the place. We're working to refactor things like this to make them more consistent on a separate branch, so I say just leave it and we'll choose the "proper" method later
There was a problem hiding this comment.
Ah I haven't been aware this was used in the same file. Maybe it's just simply lack of experience on my part that i have never seen a class instance method be called like this. The only practice I could come up with that might be somewhat relevant is that in python there should be "one, preferably obvious way of doing things" and if I had to choose I'd definitely choose variable.method(... over class.method(variable, ...
There was a problem hiding this comment.
I agree with @imsodin . I found this reference that
There is a simple way to call the base class method directly: just call BaseClassName.methodname(self, arguments).
This is the strategy used here. I see this being useful if you don't want derived classes to overwrite the sample_over_line method call inside the plot_over_line method, but you do want to be able to overwrite it in general.
I'm not sure this is what you want however.
There was a problem hiding this comment.
Regardless of whether or not it should change, that's a topic for a separate PR.
This PR enables a new filter sample_over_line, which is a simple refactoring of plot_over_line, but enables the user to retrieve the data instead of just plotting it. plot_over_line calls sample_over_line, and now only does plotting.