unyt breaking changes

[james-trayford]

I have been wrestling with SOAP issues, and realised that the most recent unyt version introduces breaking changes, such as breaking zero comparison, as in e.g.

SOAP/mask_cells.py

Line 30 in 0aec352

if done[halo_nr] == 0:

in unyt v2.9.2 this comparison yields True for a 0 dimensionless entry, but False in v2.9.4

This can be seen using unyt alone, first with unyt 2.9.4:

>>> unyt.__version__
'v2.9.4'
>>> a = unyt.unyt_array(np.zeros(5, dtype=np.int8), dtype=np.int8)
>>> for i in a:
...     print(i == 0)
... 
False
False
False
False
False
>>> 

Then with unyt 2.9.2 (latest previous version tested):

>>> unyt.__version__
'v2.9.2'
>>> a = unyt.unyt_array(np.zeros(5, dtype=np.int8), dtype=np.int8)
>>> for i in a:
...     print(i == 0)
... 
True
True
True
True
True
>>> 

comparing i.value == 0 yields Trues in both cases, but my short term suggestion is to modify requirements.txt to limit unyt<=2.9.2 as I don't know how pervasive the issue is (and know from testing this zero comparison issue comes up elsewhere).

[james-trayford]

we probably also need a minimum unyt version requirement as significantly earlier versions (e.g. v2.8.0) have no unyt.unyt_quantity.from_string() for example

[MatthieuSchaller]

@JBorrow could that break things in swiftsimio too?

Overall, unyt has some dangerous behaviours when comparing to 0 or when handling single elements. It's not a bad thing if they become more pedantic. But it means we have to be more careful too.

[james-trayford]

I did raise this on the unyt repo too, though I don't know if it is more an issue of our usage (does seem a pretty fundamental change in behaviour, however) yt-project/unyt#369

[MatthieuSchaller]

I agree that a silent change in behaviour is not great. :)

[JBorrow]

I guess this is because the 0 has dissimilar units...?

[MatthieuSchaller]

It must be.

Bert had sanitized part of our pipeline code relying on sSFR < 0.01 (with no units on the RHS) which would now likely fail.

Though if they intended that fix, throwing an exception of non-matchin units rather than returning false would be better.

[JBorrow]

Yeah, that is what usually happens when comparing to another unyt quantity, but not with constants. That would probably be the best behaviour.

[MatthieuSchaller]

(unrelated) The other gotcha we often hit is that when accessing single elements of an array no type check is done. That's dangerous. Though I have been too lazy to open an issue on their side.

[JBorrow]

Indeed and doing assignments, e.g.

x = unyt.unyt_array([0, 2, 3], "g")
y = unyt.unyt_array([5, 6, 7], "Msun")

y[1] = x[1]

print(y)
>>> [5, 2, 7] Msun

doesn't convert units...

[james-trayford]

This should fix it, was regarded as a unintentional change: yt-project/unyt#371

Can close, though maybe worthwhile discussion of how we do these comparisons? Should we open a separate issue or keep this open? Or memory-hole the whole episode?

[MatthieuSchaller]

I got motivated to list my grievances on their github...

[james-trayford]

OK! closing this...

[JBorrow]

Wait should we not list unyt==2.9.2 as an actual dependency?

[JBorrow]

Given that John appears to have fixed the core issue, I'd recommend close this.