search

scikit-hep / mplhep

Extended histogram plotting on top of matplotlib and HEP collaboration compatible styling
https://mplhep.readthedocs.io
MIT License
182 stars 63 forks source link

fix: array copy and logic for obtaining variances and values #488

Closed jonas-eschle closed 3 months ago

jonas-eschle commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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