fix: array copy and logic for obtaining variances and values

[jonas-eschle]

I've noticed the new .copy() to avoid that the underlying array changes (such as here https://github.com/scikit-hep/mplhep/blob/master/src/mplhep/plot.py#L191).

I would suggest to follow closer the array protocol, which uses the np.copy -> other, array-like types (like JAX, TensorFlow, PyTorch etc) may don't have the copy attribute but support the np.copy (as we've noticed in the CI).

Also, the logic was possibly flawed, as a histogram also has values and therefore, it would take the second if, wouldn't it? Removed also the re-assignement of 0 to the flows, they were already initialized with 0.

Does the new flow makes sense?

[andrzejnovak]

Thanks! I got a bit lazy about the copies, should have fixed that :)

Also, the logic was possibly flawed, as a histogram also has values and therefore, it would take the second if, wouldn't it?

I think this is what's tripping up the CI. hasattr(h, "axes") and hasattr(h.axes[0], "traits") uproot TH1 probably also has traits, just not the under/overflow features. It's a bit annoying to be duck-typing between hist/uproot but I don't think there's a better solution.

Removed also the re-assignement of 0 to the flows, they were already initialized with 0.

Sounds good

PS. Makes me kind of happy to see this kind of stuff caught on CI downstream

[jonas-eschle]

I think this is what's tripping up the CI. hasattr(h, "axes") and hasattr(h.axes[0], "traits") uproot TH1 probably also has traits, just not the under/overflow features. It's a bit annoying to be duck-typing between hist/uproot but I don't think there's a better solution.

true, I've refactored and re-arranged a bit. Also added a test without the variances, as this would fail.

PS. Makes me kind of happy to see this kind of stuff caught on CI downstream

yes, the advantage of a nice library that is used by many others ;)

[jonas-eschle]

Looks fine from my side, mypy fails because of the over- and underflow being arrays, not floats. Feel free to adjust, I think it was like this before as well, or let me know what's the best way to go.

Otherwise can be merged from my side

[andrzejnovak]

Huh, so the return type for np.copy(float) is apparently not float which is what was tripping up mypy. Calling copy on the array instead is probably a touch less efficient but works and we're not too concerned about performance for plotting.

Can you check if this still fine in zfit? I'll merge it then.

We could also do copy.copy for these

[jonas-eschle]

Huh, so the return type for np.copy(float) is apparently not float which is what was tripping up mypy. Calling copy on the array instead is probably a touch less efficient but works and we're not too concerned about performance for plotting.

the type returned was actually a numpy.float64, but now it's an array with shape (), afaiu. The performance should be about the same, the expensive part is anyways the copy of large array (if at all), and that happens in both cases.

Can you check if this still fine in zfit? I'll merge it then.

yes, works, can be merged

We could also do copy.copy for these

that should also work I think, alternatively, we could also convert to floats... all works, as long as we don't use the copy attribute

[andrzejnovak]

Alright, let's run with this for now, see if anything comes up laters. Thanks for the PR!