PERF: Use grid tree for counting grid cell inclusion #4529

matthewturk · 2023-06-21T15:19:40Z

Instead of using the grid objects, by default let's use the grid tree to count the number of cells included in a data object.

This is an independent PR, and may have test failures that can be corrected within this PR. It's independent of #4525, but related.

matthewturk · 2023-06-21T15:29:57Z

Looks like some real failures, which I can probably replicate here and work on!

matthewturk · 2023-06-21T18:09:30Z

Current status is that there's a difference in the slice methods for the grid tree and the grid objects, which leads to a mismatch of roughly 1/2 of the first dimension in one of the slices. Working on tracking it down.

matthewturk · 2023-06-22T18:59:29Z

I'm working on what I hope is among the last set of issues, and it's pointing to some uninitialized memory somewhere.

matthewturk · 2023-06-22T19:35:34Z

This passes locally, but answer tests may show other issues.

matthewturk · 2023-06-23T12:41:40Z

To be honest, I am a little stunned that we are seeing >1k failures on py38 but none on the unit tests.

They all follow this rough backtrace, from what I can tell:

  File "/home/fido/workspace/yt_py38_git/yt/data_objects/construction_data_containers.py", line 551, in __init__
    self.get_data(field)
  File "/home/fido/workspace/yt_py38_git/yt/data_objects/construction_data_containers.py", line 277, in get_data
    for chunk in parallel_objects(
  File "/home/fido/workspace/yt_py38_git/yt/utilities/parallel_tools/parallel_analysis_interface.py", line 538, in parallel_objects
    for result_id, obj in oiter:
  File "/home/fido/workspace/yt_py38_git/yt/data_objects/selection_objects/data_selection_objects.py", line 95, in chunks
    self.get_data()  # Ensure we have built ourselves
  File "/home/fido/workspace/yt_py38_git/yt/data_objects/selection_objects/data_selection_objects.py", line 140, in get_data
    self.index._identify_base_chunk(self)
  File "/home/fido/workspace/yt_py38_git/yt/geometry/grid_geometry_handler.py", line 350, in _identify_base_chunk
    dobj.size = self._count_selection(dobj, fast_index=self.grid_tree)
  File "/home/fido/shiningpanda/jobs/a7d58705/virtualenvs/d41d8cd9/lib/python3.8/functools.py", line 967, in __get__
    val = self.func(instance)
  File "/home/fido/workspace/yt_py38_git/yt/geometry/grid_geometry_handler.py", line 311, in grid_tree
    parent_ind[i] = grid.Parent.id - grid.Parent._id_offset
AttributeError: 'list' object has no attribute 'id'

neutrinoceros · 2023-06-23T13:27:26Z

To be honest, I am a little stunned that we are seeing >1k failures on py38 but none on the unit tests.

me too. I suspect this could be indicative of a problem with the answer test framework rather than an actual issue with your refactor, but that's just a wild (and vague) guess

matthewturk · 2023-06-23T19:57:54Z

Actually, I figured it out. It's only relevant in situations where grids can have multiple parents, but there it occurs. I'm going to address that.

matthewturk · 2023-06-23T21:01:10Z

I have to handle something else for a bit, but I just pushed a change that disabled AMRVAC using the Grid Tree and that should enable AMR types with multiple parents for a child to work. I had to do some git stashing so it's possible I missed something, but I will return to this ASAP.

I'm going to open an issue about AMRVAC, because I think we could potentially have it load the non-leaf node AMR data as well, which would allow it to use the grid tree.

matthewturk · 2023-06-23T22:34:14Z

Interestingly, passing an array as a parameter defined in a Cython argument list as int will throw an error, because of the conversion / sequence / scalar conversion.

matthewturk · 2023-08-23T18:02:53Z

Enzo-E has negative-level grids, which confused how we were computing the number of "root" grids.

matthewturk · 2023-08-25T10:13:16Z

I'm still attempting to get this working; I think I'm running into some fencepost errors. I am glad the tests are catching it quickly.

…_tree_count

neutrinoceros · 2023-09-19T06:22:14Z

@yt-fido test this please

matthewturk · 2023-09-20T13:46:17Z

That's an interesting one:

Traceback (most recent call last):
  File "/home/fido/workspace/yt_py310_git/tests/nose_runner.py", line 9, in <module>
    from yt.config import ytcfg
  File "/home/fido/workspace/yt_py310_git/yt/__init__.py", line 13, in <module>
    from yt.data_objects.api import (
  File "/home/fido/workspace/yt_py310_git/yt/data_objects/api.py", line 1, in <module>
    from . import construction_data_containers as __cdc, selection_objects as __sdc
  File "/home/fido/workspace/yt_py310_git/yt/data_objects/construction_data_containers.py", line 17, in <module>
    from yt.data_objects.selection_objects.data_selection_objects import (
  File "/home/fido/workspace/yt_py310_git/yt/data_objects/selection_objects/__init__.py", line 1, in <module>
    from .boolean_operations import (
  File "/home/fido/workspace/yt_py310_git/yt/data_objects/selection_objects/boolean_operations.py", line 5, in <module>
    from yt.data_objects.selection_objects.data_selection_objects import (
  File "/home/fido/workspace/yt_py310_git/yt/data_objects/selection_objects/data_selection_objects.py", line 32, in <module>
    from yt.utilities.lib.marching_cubes import march_cubes_grid, march_cubes_grid_flux
ImportError: /home/fido/workspace/yt_py310_git/yt/utilities/lib/marching_cubes.cpython-310-x86_64-linux-gnu.so: undefined symbol: _Z13eval_gradientPiPdS0_S0_

neutrinoceros · 2023-09-20T13:52:41Z

@matthewturk that's a flaky build error we've been seeing since Jenkins was migrated to Python 3.10 . IRCC the core problem is with parallel builds, but it seems arch-specific

…_tree_count

neutrinoceros

I got started reviewing but stopped when I stumbled upon a comment that said the code didn't work. Waiting for further instructions :)

neutrinoceros · 2023-10-06T07:48:24Z

yt/data_objects/index_subobjects/grid_patch.py

-            if not hasattr(self.Parent, "dds"):
-                self.Parent._setup_dx()
-            self.dds = self.Parent.dds.d / self.ds.refine_by
+        parents = list(always_iterable(self.Parent, base_type=(AMRGridPatch,)))


is the base_type argument necessary here ? or in other words, is AMRGridPatch iterable ?

It is indeed necessary, because AMRGridPatch exposes an iteration-like interface. This is a long-time issue, unfortunately.

neutrinoceros · 2023-10-06T07:57:17Z

yt/frontends/gdf/data_structures.py

+        for k, v in sp.items():
+            if getattr(v, "size", None) == 1:
+                sp[k] = v.item(0)


I see a couple problems here

modifying a dict while iterating over it is unsafe and may raise warnings/errors (use a copy)

using the existence of a size attribute to detect array-like objects seems a bit fragile. Could this be done with isinstance instead ? If not, I would suggest using a EAFP pattern instead of LBYL (basically, switch to try/except)

the comment also doesn't explain why this conversion is needed. It'd be more helpful if it did.

So the reason I'm using size rather than a base class is to avoid h5py as an instance; we're actually checking here if it's an HDF5 attribute array, rather than a numpy array. I'm not sure that I agree with the suggested change -- particularly because we really do want this specific check, less so than whether or not it can be accessed like a dict.

neutrinoceros · 2023-10-06T07:58:54Z

yt/frontends/stream/definitions.py

@@ -47,20 +47,21 @@ def assign_particle_data(ds, pdata, bbox):
    if len(ds.stream_handler.fields) > 1:
        pdata.pop("number_of_particles", None)
        num_grids = len(ds.stream_handler.fields)
-        parent_ids = ds.stream_handler.parent_ids
+        # We have to convert these to a list of lists


This comment seems to relate to line 58 ? If so, it should be moved closer to it.
I would also like to know the why here.

neutrinoceros · 2023-10-06T08:00:48Z

yt/geometry/grid_container.pyx

@@ -55,12 +55,23 @@ cdef class GridTree:
                  np.ndarray[np.float64_t, ndim=2] left_edge,
                  np.ndarray[np.float64_t, ndim=2] right_edge,
                  np.ndarray[np.int32_t, ndim=2] dimensions,
-                  np.ndarray[np.int64_t, ndim=1] parent_ind,
+                  list parent_ind, # Not an array anymore!


This comment will probably not age well. Also, I think Cython 3 supports more expressive uses of Python type annotations like list[list[float]], might be worth a try

Suggested change

list parent_ind, # Not an array anymore!

list parent_ind,

I didn't know that -- I'll give that a go.

yt/frontends/stream/definitions.py

yt/geometry/_selection_routines/ray_selector.pxi

Co-authored-by: Clément Robert <[email protected]>

matthewturk · 2024-04-26T18:37:13Z

I'm trying to let the tests go so we have a current version that includes #4525.

yt/data_objects/tests/test_dataset_access.py

…_tree_count

matthewturk · 2024-04-26T18:52:39Z

I think I've addressed all the comments. Unfortunately I still need to wait for the tests to pass or fail to make any more efforts to make it working :)

matthewturk · 2024-04-26T21:57:16Z

Tests all pass!

Enable using the grid tree for counting

8b3a320

matthewturk mentioned this pull request Jun 21, 2023

PERF: Use bitarray to count grid cells #4530

Draft

matthewturk added 2 commits June 21, 2023 10:49

Avoid reusing bitarrays for now

8b2c75a

Don't count selection with fast_index for grids

196a2a2

matthewturk added performance index: grid labels Jun 21, 2023

Avoid data collection

a93664a

matthewturk added the enhancement Making something better label Jun 21, 2023

Make refine_by and ref_factor work in grid trees

45deabe

matthewturk added 3 commits June 21, 2023 15:19

Special case the slice counting.

001c96e

Split pytest tests out

827bb9e

Replicate the mask filling for rays in visit_grid_cells

61235df

Avoid using 'fast_index' for ray objects

8b49103

Don't ignore access for pytest, but ignore checksums

831d4a7

matthewturk added 2 commits June 23, 2023 15:57

Disable grid tree for AMRVAC, as it only loads leafs

075db4b

Allow multiple parents for grid tree

bd55726

matthewturk added 2 commits June 23, 2023 16:32

Don't consider AMRGridPatch objects iterable

222764c

Update other invocations of GridTree

57ce059

matthewturk added 3 commits June 23, 2023 19:03

Handle non spatial hierarchy

041c9f5

Set min_level for yt data hierarchy

218467f

Set default min level to 0.

150c84b

enzo_e has negative level grids

188390a

matthewturk added 6 commits August 24, 2023 09:13

Grid Tree should only be used in 3D

8ce607f

Some more missed noexcepts

dd033e8

Some cases of arrays of refine_by are not 3D

e28e4b1

Simplify checks

9bd7462

Clip child bounds to parent for recursive descent.

301d272

Clip internal boundaries to parent.

f65df99

matthewturk added 3 commits August 25, 2023 06:06

Avoid parentage checks and use a bitmask to only visit once.

f390a2b

Scalar-ize GDF attributes

e061fa6

Merge branch 'grid_tree_count' of github.com:matthewturk/yt into grid…

e987baf

…_tree_count

Merge branch 'main' into grid_tree_count

fdca0de

Merge branch 'grid_tree_count' of github.com:matthewturk/yt into grid…

1260e4a

…_tree_count

neutrinoceros reviewed Oct 6, 2023

View reviewed changes

matthewturk and others added 5 commits January 23, 2024 10:51

This type of annotation does work!

189c39d

Merge branch 'main' of github.com:yt-project/yt into grid_tree_count

cb576e5

Merge branch 'main' of github.com:yt-project/yt into grid_tree_count

69a5f80

Merge branch 'main' of github.com:yt-project/yt into grid_tree_count

51cdcc9

Apply suggestions from code review

86e32db

Co-authored-by: Clément Robert <[email protected]>

move comment

7544934

chrishavlin reviewed Apr 26, 2024

View reviewed changes

yt/data_objects/tests/test_dataset_access.py Show resolved Hide resolved

matthewturk added 2 commits April 26, 2024 13:46

Ignore the test file

2a7cf8e

Merge branch 'grid_tree_count' of github.com:matthewturk/yt into grid…

7dc292b

…_tree_count

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PERF: Use grid tree for counting grid cell inclusion #4529

PERF: Use grid tree for counting grid cell inclusion #4529

matthewturk commented Jun 21, 2023

matthewturk commented Jun 21, 2023

matthewturk commented Jun 21, 2023

matthewturk commented Jun 22, 2023

matthewturk commented Jun 22, 2023

matthewturk commented Jun 23, 2023

neutrinoceros commented Jun 23, 2023

matthewturk commented Jun 23, 2023

matthewturk commented Jun 23, 2023

matthewturk commented Jun 23, 2023

matthewturk commented Aug 23, 2023

matthewturk commented Aug 25, 2023

neutrinoceros commented Sep 19, 2023

matthewturk commented Sep 20, 2023

neutrinoceros commented Sep 20, 2023

neutrinoceros left a comment

neutrinoceros Oct 6, 2023

matthewturk Apr 26, 2024

neutrinoceros Oct 6, 2023

matthewturk Jan 23, 2024

neutrinoceros Oct 6, 2023

neutrinoceros Oct 6, 2023

matthewturk Jan 23, 2024

matthewturk commented Apr 26, 2024

matthewturk commented Apr 26, 2024

matthewturk commented Apr 26, 2024

PERF: Use grid tree for counting grid cell inclusion #4529

Are you sure you want to change the base?

PERF: Use grid tree for counting grid cell inclusion #4529

Conversation

matthewturk commented Jun 21, 2023

matthewturk commented Jun 21, 2023

matthewturk commented Jun 21, 2023

matthewturk commented Jun 22, 2023

matthewturk commented Jun 22, 2023

matthewturk commented Jun 23, 2023

neutrinoceros commented Jun 23, 2023

matthewturk commented Jun 23, 2023

matthewturk commented Jun 23, 2023

matthewturk commented Jun 23, 2023

matthewturk commented Aug 23, 2023

matthewturk commented Aug 25, 2023

neutrinoceros commented Sep 19, 2023

matthewturk commented Sep 20, 2023

neutrinoceros commented Sep 20, 2023

neutrinoceros left a comment

Choose a reason for hiding this comment

neutrinoceros Oct 6, 2023

Choose a reason for hiding this comment

matthewturk Apr 26, 2024

Choose a reason for hiding this comment

neutrinoceros Oct 6, 2023

Choose a reason for hiding this comment

matthewturk Jan 23, 2024

Choose a reason for hiding this comment

neutrinoceros Oct 6, 2023

Choose a reason for hiding this comment

neutrinoceros Oct 6, 2023

Choose a reason for hiding this comment

matthewturk Jan 23, 2024

Choose a reason for hiding this comment

matthewturk commented Apr 26, 2024

matthewturk commented Apr 26, 2024

matthewturk commented Apr 26, 2024