BUG: fix buffer masking in cartesian geometry

[neutrinoceros]

PR Summary

Goal: fix #3854
Applying the patch from the issue. Let's see what image tests fail.

[neutrinoceros]

So far I haven't been able to localise the source of differences in the affected PixelizedProjectionValuesTests
I'm trying to reproduce something similar without the testing framework.

@matthewturk, any idea what might be causing this ?

reference

this branch

[matthewturk]

Can you do a floating point comparison? I'm not entirely sure why the masking would cause these things, except that it's possible that some things that were previously 0.0 are now NaN right at the boundary, or vice versa.

[neutrinoceros]

So I was able to reproduce this locally by running the following on the tip of main

import yt
import numpy as np
import matplotlib.pyplot as plt
from matplotlib.colors import LogNorm

field = ("gas", "temperature")
ds = yt.load("output_00080/info_00080.txt")
obj = ds.sphere("max", (0.1, "unitary"))
axis = 0
weight_field  = ("gas", "density")
proj = ds.proj(field, axis, weight_field=weight_field, data_source=obj)
frb = proj.to_frb((1.0, "unitary"), 256)
np.save(f"main_weightfield_{weight_field}.npy", frb[field].d)

followed by

git checkout cartesian_buffer_masking
pip install -e .

Then repeating the script but saving to a different file

np.save(f"branch_weightfield_{weight_field}.npy", frb[field].d)

and finally running

fig, axs = plt.subplots(nrows=1, ncols=3, figsize=(15, 4))
d1 = np.load(f"main_weightfield_('gas', 'density').npy")
d2 = np.load(f"branch_weightfield_('gas', 'density').npy")

n1 = LogNorm(vmin=np.nanmin(d1), vmax=np.nanmax(d1))
n2 = LogNorm(vmin=np.nanmin(d2), vmax=np.nanmax(d2))
diff = np.nansum((d2, -d1), axis=0)
for data, norm, ax in [
    (d1, n1, axs[0]),
    (d2, n2, axs[1]),
    (diff, LogNorm(), axs[2]),
]:
    im = ax.imshow(data, norm=norm, cmap="cmyt.arbre")
    fig.colorbar(im, ax=ax)
fig.savefig("/tmp/bench.png")

So it seems that your intuition was right Matt, the only place where there are actual differences is at the boundary of the selection, though I still do not understand why that is.
In any case it seems like more pixels get -correctly ?- colorised with this branch. So as far as I understand this looks more like a fix than a regression. I'm just not satisfied with my limited understanding of this difference.
@matthewturk should we move forward with it ? There are other places where buffers get initialised with zero as a filler, I probably should update them too.

[matthewturk]

I agree with your assertion that this is a fix.

My suspicion is that it involves the anti-aliasing argument, and that if it were turned off we would see less difference. Regardless, this looks like an improvement to me, and I say move forward.

[neutrinoceros]

So, my attempt to generalise using nan as a fill value to sph-like particle pixelization routines has resulted in multiple test failures that I can't seem to reproduce locally.
As a result, I anticipate that progressing any further in that direction is going to take much more time than I can afford at the moment. @matthewturk, do you think we could get this merged with just the grid/cartesian changes that we already discussed ?

[matthewturk]

@neutrinoceros Yes, I think that's reasonable.

[neutrinoceros]

@matthewturk this is ready

[cphyc]

Why not use the following?

Suggested change

	assert_equal(np.nanmin(p4rho[p4rho > 0.0]) >= 0.5, True)
	assert np.nanmin(p4rho[p4rho > 0.0]) >= 0.5

Note that I am absolutely happy about the PR as is and don't want to delay it any further, so feel free to push the merge button!

[neutrinoceros]

I think I’m going to keep it as is to match the local dominant convention, even though this is actually not a unit test-based test, so your suggestion should indeed work.

[cphyc]

In my comment I wrote

Note that I am absolutely happy about the PR as is and don't want to delay it any further, so feel free to push the merge button!

So that's absolutely fine by me :) 31 Mar 2022 12:00:51 Matthew Turk ***@***.***>:

…

@neutrinoceros[https://github.com/neutrinoceros] did you just merge your own PR, before @cphyc[https://github.com/cphyc] responded to your reply? — Reply to this email directly, view it on GitHub[#3856 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABJJII6KYOUNSYWA746TSUDVCWAWFANCNFSM5RI7ZGIQ]. You are receiving this because you were mentioned.[Tracking image][https://github.com/notifications/beacon/ABJJIIZV7QJ3NNLKQSYFYHLVCWAWFA5CNFSM5RI7ZGI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOICRP25Q.gif]

[matthewturk]

@cphyc I deleted my comment right after I posted, because I realized