Add masking for phase plots.

[matthewturk]

This PR does not yet work, because I need some help figuring out the right way to navigate a collision between unyt arrays and masked arrays.

Here is an example script:

import yt
ds = yt.load_sample("IsolatedGalaxy")
dd = ds.all_data()
p = dd.profile(["density", "temperature"], "velocity_x", logs = {('gas', 'velocity_x'): False}, weight_field="mass").plot()
p.set_log("velocity_x", False)
p.save("hi.png")

And before and after images:

I made these with a slight modification to this PR, which was to change the creation of masked_data to:

masked_data = np.ma.masked_array(data.v, ~self.profile.used)

(adding the .v on data)

I'm not sure that's the right way to go. If I don't strip the unyt status, at some point in the copying process the unyt machinery tries to coerce the boolean mask into units, or something, which breaks stuff.

[matthewturk]

I think with this change it should work fine.

[matthewturk]

There have been enough wonky answer-store submodule changes that I've merged and pushed that merged version of this to get a new set of tests.

[matthewturk]

I think that this is now going to be easier with the changes in unyt to support masked arrays. I will try again.

[munkm]

Hi @matthewturk did you get a chance to look at unyt's updates to see if it is more compatible for these changes now? This seems like a cool feature!

[matthewturk]

Just updated -- let's see how it does.

[Xarthisius]

@yt-fido test this please

[matthewturk]

@yt-fido test this please

[matthewturk]

Aiming to figure this out in time for 4.0.

[neutrinoceros]

closing + reopening to reactualize CI

[neutrinoceros]

This patch seems to be viable still, but I note that the reference images have changed since, namely, the lower value in the colorbar is now negative. I do not know what is the intended behaviour, is this a regression or a fix ?

[matthewturk]

@neutrinoceros The two images you show have the same colorbounds, I think?

[neutrinoceros]

They did, and still do, I'm just wondering wether negative values are expected to be displayed there.
I'm sure there are some in the dataset, but I don't know if the phase plot is supposed to show absolute values or actual velocities. In any case, the behaviour changed since you opened this, but that's not caused by your patch !

[neutrinoceros]

Anyway it looks to me that they used to be clipped to 0 (which is incorrect in any case), so I think the current behaviour is ok !

[matthewturk]

I think the root cause is related to what we are masking; previously, we were masking using the value zero. So, lots of the colors were very close to the color assigned to zero. (I suspect that using a symlog colorbar would draw this out more.) Now, we can actually see them. (And, coincidentally, the real values that were close to or equal to zero would show up, whereas before we couldn't pick them out.)

[neutrinoceros]

There's been many changes in the answer store since this was last synced. I think we could simply merge this if we can get this green first, which you should be able to do by merging from the main branch again :)

[neutrinoceros]

The one failure on Jenkins is expected, since the difference is intended.

[neutrinoceros]

Force-pushed with a rebase.

[neutrinoceros]

I am not 100% convinced these differences are expected actually.

@matthewturk, does this make any sense to you ?

[matthewturk]

One possibility is that now we are no longer using zeros anywhere, which may slightly change the colorbar calculation in vmin; if it gets a NaN it might do a different calculation than if not, in the norm.

[neutrinoceros]

I'm not convinced either way. I cannot reproduce a similar change in pure matplotlib at the moment. AFAICT, masking with 0s or NaNs seems to produce visually identical results when using a LogNorm + "nearest" shading

I'm however confident the patch is correct so I'm tempted to approve anyway, but this diff is intriguing.

[neutrinoceros]

It seems plausible that #3793 may have resolved this problem. Let's run the test suite again: close+reopen

[neutrinoceros]

This bugfix is crucial to a refactor I'm working on, so I'll be optimistic and use auto-merge and set it up for backport. 🤞🏻

[neutrinoceros]

@matthewturk I'd really like to move forward with this and I confirm that I think the new results are correct. Do you need a hand to update gold standards ?

[matthewturk]

@neutrinoceros yes please

[neutrinoceros]

Here's the supporting answer store PR yt-project/answer-store#30

and the patch needed to for Jenkins-hosted tested, in this PR

diff --git a/tests/tests.yaml b/tests/tests.yaml
index e042e5670..f7836ea15 100644
--- a/tests/tests.yaml
+++ b/tests/tests.yaml
@@ -95,7 +95,7 @@ answer_tests:
     - yt/frontends/owls/tests/test_outputs.py:test_snapshot_033
     - yt/frontends/owls/tests/test_outputs.py:test_OWLS_particlefilter

-  local_pw_041:  # PR 3670
+  local_pw_042:  # PR 2504
     - yt/visualization/tests/test_plotwindow.py:test_attributes
     - yt/visualization/tests/test_particle_plot.py:test_particle_projection_answers
     - yt/visualization/tests/test_particle_plot.py:test_particle_projection_filter