Grids (part 3) #981
Grids (part 3) #981
Conversation
- Made out-of-bound value detection explicit in interpolate_indices - Also fixed a units bug for self.grids for non-uniform grids.
Now works if you reverse the point order
|
I took the liberty of replacing your implementation of grid_resolution for the non-uniform case with a few lines of scipy code :) I also added a very basic test for it, but it'd be great if you could double-check if it still does the same thing - or, even better, add more detailed tests! ;) |
|
@StanczakDominik Thanks for the improved |
|
@StanczakDominik I was able to re-implement persistent interpolation with cached properties: the code is definitely cleaner and, based on a speed test, maybe even a hair faster! I tried to add |
Codecov Report
@@ Coverage Diff @@
## master #981 +/- ##
==========================================
- Coverage 96.68% 96.62% -0.06%
==========================================
Files 64 64
Lines 6175 6249 +74
==========================================
+ Hits 5970 6038 +68
- Misses 205 211 +6
Continue to review full report at Codecov.
|
|
Yay, I finally broke tests 😼 The codecov failure displays the tests lines that are not covered by tests in this code. It was supposed to pop up earlier, but I sort of missed that it wasn't working right for the last two months... Anyway, these are minor and let's fix them later on (#895 ?), so this doesn't get stalled further. On to the final review I go! |
There was a problem hiding this comment.
Okay, let's get this done! :) (and hopefully go back to the minor coverage leaks in #895 :) )
|
Yay, Codecov is back!! Writing tests just isn't much fun without it... |
|
Yeah... sorry for that! |
I'm getting really close to a reviewable proton radiography PR, but I ended up needing some new features (and some other minor improvements) to grids. A few of these are breaking changes, I'll try to mark them below:
Major
Introduced 'persistent interpolators' which assume the grid has not changed between calls (as long as a keyword flag is set). Otherwise, ~25% of the interpolation cycle was being wasted constantly rebuilding some arrays. I tried to make this as elegant as possible, but maybe it can be done better? Something like this has to be done, though.
Introduced the
grid.grid_resolutionmethod which determines a single scalar length scale from the grid spacing. I'm using this to determine a simulation timestep. This could be defined in the proton radiography module, but it's generally applicable so I put it here.Introduced
grid.on_grid: a method which determines which of a list of points lies within the region bounded by the grid. Useful for the proton radiography code, likely also elsewhere so I put it here.Introduced
grid.vector_intersectswhich determines whether a line between two points intersects the grid using a ray-box intersection algorithm. This is broadly useful.Minor
grid.gridandgrid.gridsare now dimensioned. Subsequently,grid.gridsis only defined for grids with the same units on all three dims.grid.pts#are still dimensioned, their API hasn't changed. (BREAKING)grid.is_uniform_gridis now justgrid.is_uniformbecause the old name was redundant. (BREAKING)Added a
grid.quantitiesproperty that returns a list of all the quantities defined on the grid.Other minor speed improvements to interpolators, none of which are breaking. For example, interpolation is now done without units and I was able to collapse some for loops into array ops.
Everything should have associated tests added
Here's an example of the speed enhancement achieved with the
persistentkeyword for 6 quantities at 5e5 positions on a (200x200x200) grid (these are times per cycle, so the extra 300 ms adds up fast).