HDFGroup / h5pyd

h5py distributed - Python client library for HDF Rest API
Other
110 stars 39 forks source link

Use == for comparisons #58

Closed kalvdans closed 5 years ago

kalvdans commented 5 years ago

You can't compare tuples with is since they will be given a new ID every time you construct a tuple.

>>> (Ellipsis,) is (Ellipsis,)
False
>>> (Ellipsis,) == (Ellipsis,)
True

The error is here in the code:

https://github.com/HDFGroup/h5pyd/blob/8f92ac12015abcd433dcbb7883ee0e8127bb7f51/h5pyd/_hl/dataset.py#L581

jreadey commented 5 years ago

Thanks for spotting that!

I made this change in a misguided attempt to fix this warning:

$ python test_dataset_getitem.py Test1DZeroFloat.test_mask filename: /home/john/h5pyd_test/dataset_test1dzerofloat.h5 /Users/jreadey/anaconda3/lib/python3.6/site-packages/h5pyd-0.3.2-py3.6.egg/h5pyd/_hl/dataset.py:581: DeprecationWarning: The truth value of an empty array is ambiguous. Returning False, but in future this will result in an error. Usearray.size > 0to check that an array is not empty. if args == (Ellipsis,) or args == tuple():

I'm not sure of the proper way to fix this though. Looks like the same code is in h5py as well.

For not I just reverted the change.

kalvdans commented 5 years ago

Thanks for a quick response! I understand now the reason for using is. It seems one first has to test the type and then the tuple contents:

isinstance(args, tuple) and args == (Ellipsis,)

Closing bug since the change was reverted. Good that you keep the code free from deprecation warnings.

jreadey commented 5 years ago

You suggestion doesn't quite work since we still get the warning -- the comparison is between args[0] and Ellipsis which triggers the numpy warning.

This revised code seems to do the trick:

# === Check for zero-sized datasets =====
if self._shape is None or numpy.product(self._shape) == 0:
     # These are the only access methods NumPy allows for such objects
     if len(args) == 0 or len(args) == 1 and isinstance(args[0], tuple) and args[0] == Ellipsis:
           return numpy.empty(self._shape, dtype=new_dtype)
kalvdans commented 5 years ago

Sorry, but keeping adding a lot of special conditions won't help in the long run. I suggest playing around a bit in an interactive Python session to find out how isinstance works. isinstance(x, tuple) is a shorthand for type(x) == tuple

jreadey commented 5 years ago

I do sort of know how ininstance works and use it in the code above.

This would be a bit cleaner I guess::

if len(args) == 0 or isinstance(args[0], tuple) and args[0] == Ellipsis:
           return numpy.empty(self._shape, dtype=new_dtype)