-
Notifications
You must be signed in to change notification settings - Fork 2
Change method: statistic from Property to Method to allow eg active.mean(axis=(0, 1)) and instriduce comprehensive testing of Reductionist axis implementation
#300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… the headers such as x-activestorage-count in favour of returning their data as part of the JSON payload.
|
@maxstack many thanks for looking into this! I think I found the issue at hand - in the current Reductionist, the response is a dict that has a "bytes" key eg: but that value comes in as raw bytes and needs to be decoded at end pount by the Client; this explains a few things:
You can see the 503 from the failed test: -> that's a json decoder error (which also, incidentally, completely destroyed the memory on my local machine). We should not get Reductionist to return raw bytes, we need actual data that can not risk corruption and the Client being unable to decode it and use. Is this something doable? Cheers 🍺 |
|
@maxstack I added a test that runs with a small file, run it like so: the test uses a small file with data 15x143x144, and looks like this: def test_small_file_axis_0_1():
"""Fails: activestorage.reductionist.ReductionistError: Reductionist error: HTTP 502: -"""
active = build_active_small_file()
result = active.min(axis=(0, 1))[:]
print("Reductionist final result", result)
assert result == [[[[164.8125]]]]the result that Reductionist returns looks like: so it looks like Reductionist is not applying a final statistic, by the looks of it - no idea why there are 101 elements in that array though; at any rate, the full payload sent by Reductionist looks like this: bytes is 576 long, so that's a bit weird too. Anyway, at least this test runs 🍺 |
|
the test above, if run locally with bog standard Numpy: import pyfive
import numpy as np
ds = pyfive.File("/home/valeriu/CMIP6-test.nc")["tas"]
minarr= np.min(ds[:], axis=(0, 1))
print(len(minarr)) # 144
print(min(minarr)) # 197.69595 |
A bit of a lengthy one, but in a nutshell:
meanis not a property anymore, but a method, soactive.mean[...]becomesactive.mean()[...]so we can pass args and kwargs, so now you canactive.mean(axis=(0, 1))[...]axis- which currently doesn't work as expected, see belowMain test case for Reductionist with axis
https://github.com/NCAS-CMS/PyActiveStorage/blob/axis_api/tests/test_real_s3_with_axes.py
Test Case 1
Test Case 2
Test Case 3
These fails are here https://github.com/NCAS-CMS/PyActiveStorage/actions/runs/20272446127/job/58211728980?pr=300