Move interpolation routines to pxd #2849

matthewturk · 2020-08-12T20:59:24Z

PR Summary

This moves some functions that are in C into a pxd file, and marks them as inline.

Why do this, you might ask? Good question! It turns out that on some compiler combinations, we got into a situation where marking the grid_traversal.pyx file as c++ resulted in a propagation of C++ that ended up with cross-C/C++ calls that were disallowed. Anyway, this fixes it!

What we should definitely keep an eye on is performance regressions.

PR Checklist

pass black --check yt/
pass isort . --check --diff
pass flake8 yt/
New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

neutrinoceros · 2020-08-12T21:25:44Z

What's the reason for the line count diff ?

matthewturk · 2020-08-12T21:27:06Z

I deleted the old .h and .c files.

Will have to figure out what's up with the grid traversal build failure on fido.

We need to have this be inside, and use pointers, so that we aren't implicitly *executing* code or using *python* code in the pxd file.

matthewturk · 2020-08-14T14:09:21Z

Real errors! Probably a mistake I made in my code, which I will address.

cphyc · 2020-09-02T18:28:31Z

There doesn't seem to be significant performance implication, at least regarding isocontours extraction (we should double check the cost for volume rendering as well).
For reference:

With this PR:

[success] 62.27% yt.data_objects.tests.test_data_containers.TestDataContainers.test_extract_isocontours: 10.9075s
[success] 9.59% yt.data_objects.tests.test_data_containers.TestDataContainers.test_to_frb: 1.6794s
[success] 9.29% yt.data_objects.tests.test_data_containers.TestDataContainers.test_derived_field: 1.6276s
[success] 5.33% yt.data_objects.tests.test_data_containers.TestDataContainers.test_to_astropy_table: 0.9341s
[success] 3.53% yt.data_objects.tests.test_data_containers.TestDataContainers.test_std: 0.6182s
[success] 3.37% yt.data_objects.tests.test_data_containers.TestDataContainers.test_write_out: 0.5897s
[success] 3.35% yt.data_objects.tests.test_data_containers.TestDataContainers.test_to_dataframe: 0.5863s
[success] 3.27% yt.data_objects.tests.test_data_containers.TestDataContainers.test_yt_data_container: 0.5737s

With master:

[success] 61.75% yt.data_objects.tests.test_data_containers.TestDataContainers.test_extract_isocontours: 11.3581s
[success] 9.35% yt.data_objects.tests.test_data_containers.TestDataContainers.test_derived_field: 1.7190s
[success] 9.21% yt.data_objects.tests.test_data_containers.TestDataContainers.test_to_frb: 1.6935s
[success] 6.55% yt.data_objects.tests.test_data_containers.TestDataContainers.test_to_astropy_table: 1.2046s
[success] 3.35% yt.data_objects.tests.test_data_containers.TestDataContainers.test_std: 0.6161s
[success] 3.29% yt.data_objects.tests.test_data_containers.TestDataContainers.test_write_out: 0.6059s
[success] 3.26% yt.data_objects.tests.test_data_containers.TestDataContainers.test_to_dataframe: 0.6002s
[success] 3.24% yt.data_objects.tests.test_data_containers.TestDataContainers.test_yt_data_container: 0.5959s

cphyc

Assuming we check that the perf were not degraded, I'm all in!

matthewturk · 2020-09-02T18:32:02Z

@cphyc thanks for the fix!

I'm game for this going in; the perf does not seem to be a big deal, and is likely swallowed up by perf implications of other things that could be improved.

cphyc · 2020-09-02T18:33:14Z

@cphyc thanks for the fix!

I'm game for this going in; the perf does not seem to be a big deal, and is likely swallowed up by perf implications of other things that could be improved.

Have you tried volume rendering?

matthewturk · 2020-09-02T18:34:49Z

Yes, but I did not write down the numbers. Will attempt to do some perf comparisons asap.

cphyc · 2020-09-02T20:09:18Z

Yes, but I did not write down the numbers. Will attempt to do some perf comparisons asap.

yt.visualization.volume_rendering.tests.test_mesh_render.test_surface_mesh_render goes from 3.8746s to 7.9495s with this PR!

matthewturk · 2020-09-02T20:10:22Z

oof, ok.

…

On Wed, Sep 2, 2020 at 3:09 PM Corentin Cadiou ***@***.***> wrote: Yes, but I did not write down the numbers. Will attempt to do some perf comparisons asap. yt.visualization.volume_rendering.tests.test_mesh_render.test_surface_mesh_render goes from 3.8746s to 7.9495s with this PR! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2849 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO7ZYETNSTP7W6BOQUTSD2Q7ZANCNFSM4P5JBJCQ> .

cphyc · 2020-09-02T20:13:54Z

Yes, but I did not write down the numbers. Will attempt to do some perf comparisons asap.

yt.visualization.volume_rendering.tests.test_mesh_render.test_surface_mesh_render goes from 3.8746s to 7.9495s with this PR!

Ignore this comment, I just reran it with completely different results.

matthewturk · 2020-09-11T14:15:23Z

So maybe it's OK to go?

cphyc · 2020-09-11T14:18:24Z

I'm ok in principle, but I would like to have some robust performance test (esp. for volume rendering) before merging.

matthewturk · 2020-09-11T14:18:50Z

@cphyc fair! OK I'll work on that.

matthewturk · 2020-09-15T17:47:41Z

@cphyc I'm looking at this again and I'm not 100% convinced I still want it with the changes you made, but I'm going to add the perf anyway.

…ation_pxd

neutrinoceros · 2020-09-16T05:48:07Z

@yt-fido test this please

matthewturk · 2020-09-23T18:57:25Z

I had to commit this before I completed it; I'm also finding some strange memory things that I need to convince myself are not real before I proceed.

neutrinoceros · 2020-09-25T15:55:53Z

yt/utilities/lib/tests/test_interpolation_routines.py

+    for i in range(3):
+        random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8")


Suggested change

for i in range(3):

random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8")

for i, s in enumerate(shape):

random_index[:, i] = np.random.randint(0, s, size=M, dtype="u8")

neutrinoceros · 2020-09-25T15:55:57Z

yt/utilities/lib/tests/test_interpolation_routines.py

-    random_index = np.random.randint((0, 0, 0), shape, size=(M, 3), dtype="u8")
+    random_index = np.empty((M, 3), dtype="u8")
+    for i in range(3):
+        random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8")
    random_offset = np.random.random((M, 3))
    # import time


@matthewturk could we clean up the commented code here while we're at it ?

matthewturk · 2020-09-25T15:58:34Z

Well, I intentionally left it in so that we could compare these when I stuck it into master. So I will, but later.

…

On Fri, Sep 25, 2020 at 10:56 AM Clément Robert ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In yt/utilities/lib/tests/test_interpolation_routines.py <#2849 (comment)>: > + for i in range(3): + random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8") ⬇️ Suggested change - for i in range(3): - random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8") + for i, s in enumerate(shape): + random_index[:, i] = np.random.randint(0, s, size=M, dtype="u8") ------------------------------ In yt/utilities/lib/tests/test_interpolation_routines.py <#2849 (comment)>: > @@ -9,7 +9,9 @@ def test_fast_interpolate(): N = 10000 M = 1000 random_values = np.random.random(shape) - random_index = np.random.randint((0, 0, 0), shape, size=(M, 3), dtype="u8") + random_index = np.empty((M, 3), dtype="u8") + for i in range(3): + random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8") random_offset = np.random.random((M, 3)) # import time @matthewturk <https://github.com/matthewturk> could we clean up the commented code here while we're at it ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2849 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO27WL5PJUWVI2BCWVDSHS4SFANCNFSM4P5JBJCQ> .

neutrinoceros · 2020-11-25T12:28:15Z

@matthewturk I'm waiting for your answer but I'm ready to approve+merge this !

matthewturk · 2020-11-25T15:29:57Z

I actually spent some trying to compare, and then I forget what happened. I will attempt again.

neutrinoceros · 2020-12-17T12:30:42Z

This is starting to drift into the conflict realm.

matthewturk · 2020-12-17T15:08:54Z

Yes, it is, but I'm still struggling to get the micro-benchmarks. Won't be too hard to harmonize...

matthewturk · 2021-05-18T19:35:33Z

I'm converting this to draft until after 4.0 goes out so that it won't show up in my filters. The performance tests are harder than I expected.

neutrinoceros · 2022-07-13T12:12:58Z

@matthewturk do you want to pick this up again for the 4.1 release or should we take it out of the milestone ?

matthewturk · 2022-07-13T12:15:06Z

Let's take it out.

Move interpolation routines to pxd

80bb222

matthewturk added refactor improve readability, maintainability, modularity viz: volume rendering labels Aug 12, 2020

Xarthisius and others added 2 commits August 13, 2020 09:22

Remove obsolete 'sources' directive

ea3dd30

I thought I'd made this change, but guess not.

90ada52

We need to have this be inside, and use pointers, so that we aren't implicitly *executing* code or using *python* code in the pxd file.

Use more explicit initialization of "cverts"

5b56781

cphyc previously approved these changes Sep 2, 2020

View reviewed changes

matthewturk added 2 commits September 15, 2020 12:54

Merge branch 'master' of github.com:yt-project/yt into fixed_interpol…

96fdfb2

…ation_pxd

Removing the cpp file

48927e3

First pass at timing

f6ebd19

Do the randint in a more obvious way

be1b86a

neutrinoceros reviewed Sep 25, 2020

View reviewed changes

Base automatically changed from master to main January 20, 2021 15:27

matthewturk marked this pull request as draft May 18, 2021 19:35

neutrinoceros added this to the 4.1 milestone May 18, 2021

matthewturk dismissed cphyc’s stale review via be1b86a November 1, 2021 16:17

neutrinoceros removed this from the 4.1.0 milestone Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move interpolation routines to pxd #2849

Move interpolation routines to pxd #2849

matthewturk commented Aug 12, 2020

neutrinoceros commented Aug 12, 2020

matthewturk commented Aug 12, 2020

matthewturk commented Aug 14, 2020

cphyc commented Sep 2, 2020

cphyc left a comment

matthewturk commented Sep 2, 2020

cphyc commented Sep 2, 2020

matthewturk commented Sep 2, 2020

cphyc commented Sep 2, 2020

matthewturk commented Sep 2, 2020 via email

cphyc commented Sep 2, 2020

matthewturk commented Sep 11, 2020

cphyc commented Sep 11, 2020

matthewturk commented Sep 11, 2020

matthewturk commented Sep 15, 2020

neutrinoceros commented Sep 16, 2020

matthewturk commented Sep 23, 2020

neutrinoceros Sep 25, 2020

neutrinoceros Sep 25, 2020

matthewturk commented Sep 25, 2020 via email

neutrinoceros commented Nov 25, 2020

matthewturk commented Nov 25, 2020

neutrinoceros commented Dec 17, 2020

matthewturk commented Dec 17, 2020

matthewturk commented May 18, 2021

neutrinoceros commented Jul 13, 2022

matthewturk commented Jul 13, 2022

		for i in range(3):
		random_index[:, i] = np.random.randint(0, shape[i], size=M, dtype="u8")

Move interpolation routines to pxd #2849

Are you sure you want to change the base?

Move interpolation routines to pxd #2849

Conversation

matthewturk commented Aug 12, 2020

PR Summary

PR Checklist

neutrinoceros commented Aug 12, 2020

matthewturk commented Aug 12, 2020

matthewturk commented Aug 14, 2020

cphyc commented Sep 2, 2020

cphyc left a comment

Choose a reason for hiding this comment

matthewturk commented Sep 2, 2020

cphyc commented Sep 2, 2020

matthewturk commented Sep 2, 2020

cphyc commented Sep 2, 2020

matthewturk commented Sep 2, 2020 via email

cphyc commented Sep 2, 2020

matthewturk commented Sep 11, 2020

cphyc commented Sep 11, 2020

matthewturk commented Sep 11, 2020

matthewturk commented Sep 15, 2020

neutrinoceros commented Sep 16, 2020

matthewturk commented Sep 23, 2020

neutrinoceros Sep 25, 2020

Choose a reason for hiding this comment

neutrinoceros Sep 25, 2020

Choose a reason for hiding this comment

matthewturk commented Sep 25, 2020 via email

neutrinoceros commented Nov 25, 2020

matthewturk commented Nov 25, 2020

neutrinoceros commented Dec 17, 2020

matthewturk commented Dec 17, 2020

matthewturk commented May 18, 2021

neutrinoceros commented Jul 13, 2022

matthewturk commented Jul 13, 2022