Needs a full docstring. I think the original name is a bit better. Maybe change the signature to (x, y, func, hasval='nan'). Do we really need both hasval and func?

func does not always have a name (used with a lambda below to test for +/- inf). I'll change to func_chk_same_pos.

I don't understand this function - can't it just be written return x[~flag], y[~flag]?

No, x or y can be a scalar while the other is an array. This is why the flags were treated separately before.

The issue here is avoiding arr_0d[bool_nd], right? Or is it about avoiding non_generic_scalar[bool_0d]?

Would x, y = np.broadcast_arrays(x, y) be off the table?

Not crazy, but I'd rather stick with what I have here...

Seems like micro-optimization here - can't we just write in place of these 7 lines:

to_remove = xy_isinf | xy_isnan
x = x[~to_remove]
y = y[~to_remove]

Doesn't work for the same reason.

The problem was that xy_isinf could just be False, in which case the flags would not broadcast correctly. But of course, this is easily checked in remove_flagged, so I did that now.

nit: I'd use [] here for a size-0 array - I'm used to seeing () for a 0dim array, so this is a little jarring

OK

python or numpy scalar?

numpy, I'll add to the comment.

Can we spell this check instead of chk? Or even assert_same_predicate_pos?

OK, assert_same_pos

I think this is backward - you should return the scalar boolean mask here, not the array one (which would at this point be equal to the scalar at every position anyway). That allows you to remove all of your special cases in remove_flagged (and replace it with the one-liner I suggest below)

Yes, indeed, you are completely right - that gets rid of remove_flagged altogether. Now implemented.

np.isposinf?

Learn something new every day...

But doesn't work for complex numbers...

Does what you have here work for complex numbers?

Don't forget to remove this function now

Isn't this attribute guaranteed to exist? As long as x and y are numpy scalars, then you don't need this guard at all - scalar[bool_scalar] works just fine.

Plus then x and y are always 1d, so you can get rid of the isinstance(val, bool) and ravel() below too

No, it can be False -- initialized as such. I tried changing, but cannot get rid of the if flagged stanza, nor of the isinstance - I suggest we leave this to another PR...

Just initialize it to flagged = np.False_, to at least eliminates the getattr). I'd then spell that if flagged.ndim != 0 for clarity

I thought about that, but I do feel that I'm then decreasing the clarity of the rest of the code - at least in my emacs, False is coloured very nicely, and false_ is not.

I'd argue that keeping a variable type consistent is more important than having the color look like in your editor.

I tried changing, but cannot get rid of the if flagged stanza

I'm gonna do a checkout to see what I'm missing here

How about bool_(False), which meets both requirements?

Ok, I see why the 0d case still has special handling - I'd forgotten that arr[bool_0d].shape == (?,) + arr.shape, and is not 1D.

Or an alternate spelling - np.ndim(flagged) == 0

Yes, I like the ndim - that is much clearer why we do it. Pushed up.

nit: I think this is clearer as x_id.ndim == 0 rather than not x_id.shape

Yes, done.

Are these two lines needed? Aren't they covered by the below?

You'd think not, but numerous recursion failures if I leave them out...

So, what happens is that it does recurse, but the if statement prevents it from doing it more than once.

which is simple to solve - just use any - will push fix up shortly.

Doesn't work either, but passing in equal_[nan|inf]=False does.

No, it doesn't...

Maybe add a comment saying that this is needed to prevent recursion or something?

Good to go either way

Can you move the cast to bool to the scalar case below?

I needed to cast both x_id and y_id, but that can indeed be done at the end. That does look a bit nicer, so done.

Can you describe in the commit message why these changes were needed?

Done. This was indeed a non-trivial one - I had to look again to remind me why it was needed. I wondered about the use of stating it in the commit message until I recalled that with git blame one can find why this was added.

Probably clearer as just a normal if / else chain at this point.

Agreed; done.

Not sure I agree with this - what if x and y are quantities of different dimensions? The test should still compare the empty arrays so that the correct error is raised, right?

It is the same case as the if x.size == 0 stanza above, but for the case where x and y are both scalar. In principle, I agree the comparison should work for empty elements, but in practice, this is not the case (e.g., matrix changing its shape being one problem...)

Since this was here before, I propose to leave it as is.

Sure, i agree this is out of scope for the patch

Overall, it sadly remains a chunk of rather fragile code...

Hmm,

In [10]: ma.masked != True
Out[10]: masked

In [11]: ma.masked == True
Out[11]: masked

In [12]: bool(ma.masked)
Out[12]: False

@mhvk The camparison to True seems to achieve nothing here.

The purpose is to ensure that the stanza is not executed if x_id or y_id is masked, yet is executed if they are different.