Improve pyvista.StructuredGrid example #4365
Conversation
DocString of class StructuredGrid is updated to show that `indexing='ij'` option should be applied to prepare `x, y, z = np.meshgrid(xrng, yrng, zrng, indexing='ij')` . Details on this document modification can be found at #4360
Codecov Report
@@ Coverage Diff @@
## main #4365 +/- ##
=======================================
Coverage 95.81% 95.82%
=======================================
Files 97 97
Lines 20814 20863 +49
=======================================
+ Hits 19942 19991 +49
Misses 872 872 |
tkoyama010
added
documentation
tkoyama010
changed the title
There was a problem hiding this comment.
Thank you for this fix and clarification, @natsuwater! Would you please take some extra time to fix this everywhere you can? np.meshgrid is used in many examples and docs improperly with StructuredGrid, and we may need to fix this in more places.
|
Most of the examples have uniform X and Y coordinates, so it hasn't been much of an issue -- we should be more explicit with this though. |
akaszynski
changed the title
|
@banesullivan OK, I'll take time to look into them. |
The original code used `x[::-1]`, `y[::-1]` to reverse the mesh corrdinates. I do not figure out the purpose of this. I've just simplified the code to use `x, y, z`
x-axis is the fastest changing axis with indexing='ij', while the default indexing='xy' for np.meshgrid makes y-axis as the fastest changing axis and we preferre to have x-y-z ordering.
x-axis is the fastest changing axis with indexing='ij', while the default indexing='xy' for np.meshgrid makes y-axis as the fastest changing axis and we preferre to have x-y-z ordering.
This might not be necessary here in features.py. But I just added option indexing='ij' for overall consistency.
Improve pyvista.StructuredGrid example and docs
There was a problem hiding this comment.
Added indexing='ij' where three arguments are passed to np.meshgrid. Most of them are docstring and .rst, while features.py is an active code, and this change might not be necessary.
Note that I did not touched anything with 'test_' prefix though I find some test includes calls to np.meshgrid.
Koyama-san asked me to add an explanation about the changes in docstring. (#4365 (comment)) I'm not sure where to put the explanation for this change in the docstring of function `voxelize`, because this particular change is about internal behavior of the function `voxelize`. So I added comment on line 72 and 73. Possible position for this explanation might be line 13, before Parameters block of the docstring, but I'm afraid this explanation does not fit in docstring. I'll rewrite these on further request. But, would you please review this version. Thank you. x, y, z = np.meshgrid(x, y, z, indexing='ij')
Perfect! Thank you for taking the extra time to do this!
That's fine -- we/I should look over these for a sanity check but if the tests are passing, I wouldn't worry too much about these |
Addition of a docstring note for the change in indexing order (`indexing='ij') of numpy.meshgrid in the helper function `voxelize`, following the discussion of pull request #4365 .
|
@banesullivan and @tkoyama010, I think this is good to go. |
DocString of class StructuredGrid is updated to show that
indexing='ij'option should be applied to preparex, y, z = np.meshgrid(xrng, yrng, zrng, indexing='ij').Details on this document modification can be found at #4360
Overview
Adding option
indexing='ij'in np.meshgrid call makes the resulting SturucturedGrid(s, y, z) scans x-y-z order as RectilinearGrid does.By default, np.meshgrid uses indexing='xy', and this result in y-x-z ordering in StructuredGrid(x, y, z).