Start wprp rpbins at 0.0 #22

JosephWick · 2024-06-13T21:51:44Z

mostly a note to self

We're doing this so that sigma and wprp kernels have consistent input

the cpu serial wprp computation in Main does not take in 0.0 for the first rpbin, and does not use 0.0 anywhere
the mpi cuda wprp computation in Main does not take in 0.0, but adds it for pair counting

So, I think that we can have serial take in 0.0 and just ignore it, and then have mpi kernels take it in and return rpbins without 0.0 for the normalization math in mpi/wprp.py

However, this means that our wprp shape is len(rpbins) - 2 which seems questionable

beckermr

Thank you so much for putting this together. I am sorry I did not get a chance to do it myself.

I made some small comments on code documentation and removing some dead/duplicated code.

beckermr · 2024-06-18T17:08:21Z

diffsmhm/diff_stats/cpu/tests/test_wprp.py

@@ -13,8 +13,9 @@
 @pytest.mark.mpi_skip
 def test_wprp_serial_cpu_smoke():
    data = gen_mstar_data(seed=42)
+    data["rp_bins"] = np.concatenate([np.array([0]), data["rp_bins"]], dtype=np.float64)


We should put this change directly in the gen_mstar_data function.

beckermr · 2024-06-18T17:08:29Z

diffsmhm/diff_stats/cpu/tests/test_wprp.py

@@ -37,6 +38,7 @@ def test_wprp_serial_cpu_smoke():
 @pytest.mark.mpi_skip
 def test_wprp_serial_cpu_derivs():
    data = gen_mstar_data(seed=42)
+    data["rp_bins"] = np.concatenate([np.array([0]), data["rp_bins"]], dtype=np.float64)


Same as above.

beckermr · 2024-06-18T17:10:18Z

diffsmhm/diff_stats/cpu/wprp.py

    n_grads = w1_jac.shape[0]
-    n_rp = rpbins_squared.shape[0] - 1
+    n_rp = rpbins_squared.shape[0] - 2


So a comment in the code that the -2 is from the fact that rp_bins are the bin edges so it is +1 relative to the number of bins, and then we add zero so that is another +1, which results in us having to subtract 2, would be good to add.

beckermr · 2024-06-18T17:10:49Z

diffsmhm/diff_stats/cuda/tests/test_wprp.py

@@ -26,8 +26,9 @@ def test_wprp_serial_cuda_smoke():
    xp = get_array_backend()

    data = gen_mstar_data(seed=42)
+    data["rp_bins"] = np.concatenate([np.array([0]), data["rp_bins"]], dtype=np.float64)


Again this should be in gen_mstar_data.

beckermr · 2024-06-18T17:10:57Z

diffsmhm/diff_stats/cuda/tests/test_wprp.py

@@ -56,6 +57,7 @@ def test_wprp_serial_cuda():
    xp = get_array_backend()

    data = gen_mstar_data(seed=42)
+    data["rp_bins"] = np.concatenate([np.array([0]), data["rp_bins"]], dtype=np.float64)


Same as above.

beckermr · 2024-06-18T17:11:15Z

diffsmhm/diff_stats/cuda/tests/test_wprp.py


-    nrp = data["rp_bins"].shape[0] - 1
+    nrp = data["rp_bins"].shape[0] - 2


A comment here on the -2 as above would be helpful.

beckermr · 2024-06-18T17:12:16Z

diffsmhm/diff_stats/cuda/wprp.py


    n_grads = w1_jac.shape[0]
-    n_rp = _rpbins_squared.shape[0] - 1
+    n_rp = rpbins_squared.shape[0] - 1


We need a comment here on why this line is not -2.

beckermr · 2024-06-18T17:12:33Z

diffsmhm/diff_stats/cuda/wprp.py

@@ -201,14 +200,14 @@ def wprp_serial_cuda(
        sums = np.array(sums.get())

    # convert to differential
-    n_rp = rpbins_squared.shape[0] - 1
+    # n_rp = rpbins_squared.shape[0] - 1


Here and below there is commented out code that needs to be removed.

beckermr · 2024-06-18T17:13:03Z

diffsmhm/diff_stats/cuda/wprp.py

-    assert not xp.allclose(rpbins_squared[0], 0)
+    for rpb in rpbins_squared:
+        assert xp.allclose(rpb[0], 0)
+    """


We can remove this commented code block if we do not need it.

Joseph Wick and others added 13 commits June 13, 2024 21:50

first commit to wprp bin start at 0

94e7ed2

update assertion for list inputs

8e0a1a2

update serial check rpbin

fe39d58

flake compliance

598d99b

update indexing to expect rpbin[0] == 0

2e7e490

update rpbins for serial cpu test

6bc2b19

fix key name

7a1a7aa

Fix wprp cpu serial to have same result with as prior to 0.0 req

6d87c80

flake compliance

af0b4f0

add 0.0 first bin functionality and update test

27b78a4

update cpu kernel for rpbin[0] == 0.0

fe02712

add 0.0 to rpbins for cpu kernel test

97ba4a7

clean up code; remove prints

4467c7b

JosephWick marked this pull request as ready for review June 18, 2024 01:08

beckermr requested changes Jun 18, 2024

View reviewed changes

Joseph Wick and others added 4 commits June 19, 2024 22:22

move concatenate to gen_mstar_data

4c8541f

remove commented out code

cefdb75

add comments for rpbins sizing and n bins

b56b259

Merge branch 'Optimize-wprp' into wprp_zero_start

e0d4964

JosephWick merged commit 7075bbd into Optimize-wprp Jul 1, 2024
1 check passed

JosephWick deleted the wprp_zero_start branch July 1, 2024 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start wprp rpbins at 0.0 #22

Start wprp rpbins at 0.0 #22

JosephWick commented Jun 13, 2024 •

edited

Loading

beckermr left a comment

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024

beckermr Jun 18, 2024


		nrp = data["rp_bins"].shape[0] - 1
		nrp = data["rp_bins"].shape[0] - 2

Start wprp rpbins at 0.0 #22

Start wprp rpbins at 0.0 #22

Conversation

JosephWick commented Jun 13, 2024 • edited Loading

beckermr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephWick commented Jun 13, 2024 •

edited

Loading