BUG: fix unit comparison for K VS degC

[neutrinoceros]

This resolves the first problem in #407
It seems to break other tests (nothing that seems unfixable though), so I thought I'd expose early in case anyone wants to provide feedback

[ngoldbaum]

This change seems fine in principle, thanks for catching it and working on a fix.

That said, we have to be super careful about performance since this is a hot path. I would suggest doing some %timeit benchmarks (or something fancier if you wish). Unfortunately I'm pretty sure the pyperf benchmarks in the repo have bitrotted at this point so they might not be the best place to start.

Also one thing to keep in mind when doing benchmarks that depend on sympy is to install the fastcache module, which implements a fast C LRU cache that sympy will use if it's installed.

[ngoldbaum]

I don't remember exactly why but I'm pretty sure there was a reason __ne__ wasn't implemented in terms of ==. Speed maybe? Avoids one extra function call and the indirection of going from == to __eq__ in CPython.

If there isn't a big speed difference and you do end up doing it this way there's no need to keep the rest of the implementation of __ne__.

[ngoldbaum]

The dimension comparison should come last, for performance reasons.

[neutrinoceros]

Thanks @ngoldbaum for this early review !
Two,very similar tests are currently failing, that raise a FutureWarning, see

unyt/unyt/tests/test_unyt_array.py

Lines 2470 to 2480 in fd98eea

	def test_delta_degC():
	t1 = 10 * degC
	t2 = 1 * K
	assert t1 + t2 == 11 * degC
	# In the current implementation 1*K + 10*degC = 11*K
	# In the future, this will generate UnitOperationError
	with pytest.warns(FutureWarning):
	t2 + t1
	assert t2 + t1 == 11 * K
	t3 = 1 * delta_degC
	assert t1 + t3 == 11 * degC

Am I understanding correctly that 1K + 1degC is deprecated but 1degC + 1K isn't ?
In any case, I missed this deprecation in #402, should it be expired for unyt 3.0 ?

[ngoldbaum]

Am I understanding correctly that 1K + 1degC is deprecated but 1degC + 1K isn't

Yes, because 1 K is the same as 1 delta_degC, so treating it as a temperature delta is well-defined.

Good call on the deprecation, it’s probably worth making it raise an error.

TBH temperature units are such a pain I kinda wish we had stuck with just supporting Kelvin but that ship sailed a long time ago.

[neutrinoceros]

Bugs have to come from somewhere though :D
I'll make a separate (hopefully easier to ship) PR to expire the deprecation (edit: for posterity, this is #410)

[neutrinoceros]

I've revamped this.

• it's now rebased on top of DEPR: expire deprecation for mixed temperature units in ufuncs #410
• tests pass
• I've taken performances considerations into account. I ended up tossing Unit.__ne__ completely, in favour of Unit.__eq__, so hopefully every bit of optimisation and logic is implemented just once and reused.

[neutrinoceros]

There's now a new error in doc tests, where the results of adding two scalar temperatures with dtype == int64 has dtype == float64, so the repr is a little different. This is actually consistent with how other unitful scalars have been behaving for a while, so I think the correct fix is to just change the test's expectation, but maybe I'm misguided, so I'll wait for inputs on this point.

[ngoldbaum]

Thanks so much for looking closer at this! Changing the doctest is fine.

[neutrinoceros]

rebased on main now that #410 is merged

[ngoldbaum]

Just one issue otherwise LGTM

[ngoldbaum]

I'm pretty sure this last conditional will always be True because delta_degF and delta_degC have base_offset=0.0.

[neutrinoceros]

the second condition in on u1 while this one is on u0. I don't like it very much, I think I reached the point where I was more or less typing random conditions until I got one that passed tests, but I can guarantee that it's needed :)