Fix some todos #1646

dota17 · 2020-08-21T04:01:50Z

Fix some legacy TODOs.

codecov · 2020-08-21T04:17:57Z

Codecov Report

Merging #1646 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1646      +/-   ##
==========================================
+ Coverage   88.51%   88.52%   +0.01%     
==========================================
  Files          17       17              
  Lines        2255     2258       +3     
==========================================
+ Hits         1996     1999       +3     
  Misses        259      259

Impacted Files	Coverage Δ
h5py/_hl/group.py	`96.80% <ø> (ø)`
h5py/_hl/attrs.py	`95.52% <100.00%> (+0.10%)`	⬆️
h5py/_hl/dataset.py	`93.15% <0.00%> (ø)`
h5py/_hl/filters.py	`92.78% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fba98fd...0bde4a2. Read the comment docs.

h5py/_hl/group.py

aragilar · 2020-08-21T07:10:29Z

h5py/h5p.pyx

-                src_dset_name = bytes(name).decode('utf-8')
+                len = H5Pget_virtual_dsetname(self.id, index, name, <size_t>size+1)
+                if len > 0:
+                    src_dset_name = bytes(name).decode('utf-8')


What happens when len == 0? What's returned?

oh, the docs said: ~~Returns a non-negative value if successful; otherwise returns a negative value.~~ Returns the length of the dataset name if successful; otherwise returns a negative value.,
so, should we do it like this: if len >= 0?

The issue is around src_dset_name possibly not being defined, and should we have a fallback value or an error or do something else?

Yes, we should do something when an error is detected.

@aragilar should we tackle it like this:

https://github.com/HDFGroup/hdf5/blob/e54f5a14d28cf9eb1226ce7feab2e2600d3984b4/java/src/jni/h5pDCPLImp.c#L1286-#L1300

Negative (error) return values are already checked and converted to Python exceptions in the autogenerated function wrappers (look in defs.pyx). So there's no need to handle them here.

Do we need a special case when there are 0 bytes? It may not make sense to have an empty string, but if that's what HDF5 gives us with no error, we can convert it to an empty Python string without introducing any ambiguity or inaccuracy.

So it's not clear to me what these three TODOs are for. Maybe we can just remove the TODOs and leave the code alone.

h5py/tests/test_dataset.py

aragilar · 2020-08-21T07:16:48Z

The order tracking looks good to me (though the require_group bit I'm unsure about, given the issues around require_dataset).

I'm less certain around the cython string checking, given src_dset_name is defined within an if scope (I'm not sure that case is well tested).

h5py/_hl/group.py

takluyver

TODOs in code can be surprisingly tricky - they often mean it's not clear how something should be done, in which case it's not easy to fix them without a fair bit of discussion.

takluyver · 2020-08-30T11:34:38Z

h5py/_hl/dataset.py

+    @property
+    @with_phil
+    def track_order(self):
+        """ Whether dataset creation order are tracked (T/F)"""


Suggested change

""" Whether dataset creation order are tracked (T/F)"""

""" Whether attribute creation order is tracked (T/F)"""

takluyver · 2020-08-30T11:44:41Z

h5py/_hl/dataset.py

@@ -518,6 +518,20 @@ def fletcher32(self):
        """Fletcher32 filter is present (T/F)"""
        return 'fletcher32' in self._filters

+    @property
+    @with_phil
+    def track_order(self):


As this method specifically relates to attributes, I think it would make more sense to expose it on the AttributeManager class, so you access something like ds.attrs.order_tracked.

This also conveniently makes the interface the same for datasets, groups, and named datatypes, all of which can have attributes with or without this setting.

takluyver · 2020-08-30T11:56:36Z

h5py/_hl/dataset.py

+    def track_times(self):
+        """ Whether times associated with an object are tracked (T/F)"""
+        dcpl = self._dcpl
+        return dcpl.get_obj_track_times()


I don't think it makes sense to expose this in the high-level API when there's no high-level way to access the timestamps (the low-level way is with h5o.get_info()).

This wasn't the first time exposing get_obj_track_times in the high level API, it comes from here

h5py/h5py/_hl/group.py

Line 263 in 15ed70a

kwupdate.setdefault('track_times', dcpl.get_obj_track_times())

so I just move it into a method, and let these properties stay at the same place.

takluyver · 2020-08-30T12:14:42Z

h5py/_hl/group.py

+            else:
+                order = True
+            if not order == track_order:
+                raise TypeError("tack_order do not match (existing %s vs new %s)" % (order, track_order))


It's not clear to me if this kind of check makes sense. require_dataset() checks shape & dtype - the fundamental components of what a dataset is - but all of the extra parameters (including track_order) are only used if it creates a new dataset.

I think it's pragmatic for details like this to be unchecked - you can tell require_* how to create an object if necessary, but still use an existing object even if it doesn't match all your preferences. Nobody is rushing to 'fix' the checks in require_dataset(), and it's nearly a year since I suggested on #897 that we leave it as-is.

well, I will leave it as-is, just remove this kind of TODO.

takluyver · 2020-08-30T12:26:45Z

h5py/h5p.pyx

-                src_dset_name = bytes(name).decode('utf-8')
+                len = H5Pget_virtual_dsetname(self.id, index, name, <size_t>size+1)
+                if len > 0:
+                    src_dset_name = bytes(name).decode('utf-8')


Negative (error) return values are already checked and converted to Python exceptions in the autogenerated function wrappers (look in defs.pyx). So there's no need to handle them here.

Do we need a special case when there are 0 bytes? It may not make sense to have an empty string, but if that's what HDF5 gives us with no error, we can convert it to an empty Python string without introducing any ambiguity or inaccuracy.

So it's not clear to me what these three TODOs are for. Maybe we can just remove the TODOs and leave the code alone.

dota17 · 2020-09-11T07:50:39Z

rerun to pass static-check.

Fix some todos

477a498

aragilar reviewed Aug 21, 2020

View reviewed changes

h5py/_hl/group.py Show resolved Hide resolved

aragilar reviewed Aug 21, 2020

View reviewed changes

h5py/tests/test_dataset.py Outdated Show resolved Hide resolved

dota17 commented Aug 21, 2020

View reviewed changes

h5py/_hl/group.py Show resolved Hide resolved

dota17 added 2 commits August 24, 2020 11:36

update test_dataset.py

86752d3

update group.py/test_group.py

a5e958d

dota17 requested a review from aragilar August 24, 2020 07:05

takluyver reviewed Aug 30, 2020

View reviewed changes

dota17 added 3 commits September 7, 2020 15:40

revert some changes

aa3eb0d

add method order_tracked() in attrs.py

6d124f4

test order_tracked() method

4621f4f

dota17 closed this Sep 11, 2020

dota17 reopened this Sep 11, 2020

fix TODO about family driver test

0bde4a2

dota17 requested a review from takluyver September 14, 2020 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some todos #1646

Fix some todos #1646

dota17 commented Aug 21, 2020

codecov bot commented Aug 21, 2020 •

edited

aragilar Aug 21, 2020

dota17 Aug 21, 2020 •

edited

aragilar Aug 21, 2020

dota17 Aug 21, 2020

dota17 Aug 24, 2020

takluyver Aug 30, 2020

aragilar commented Aug 21, 2020

takluyver left a comment

takluyver Aug 30, 2020

takluyver Aug 30, 2020

takluyver Aug 30, 2020

dota17 Sep 7, 2020

takluyver Aug 30, 2020

dota17 Sep 7, 2020

takluyver Aug 30, 2020

dota17 commented Sep 11, 2020

	""" Whether dataset creation order are tracked (T/F)"""
	""" Whether attribute creation order is tracked (T/F)"""

Fix some todos #1646

Are you sure you want to change the base?

Fix some todos #1646

Conversation

dota17 commented Aug 21, 2020

codecov bot commented Aug 21, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

dota17 Aug 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aragilar commented Aug 21, 2020

takluyver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dota17 commented Sep 11, 2020

codecov bot commented Aug 21, 2020 •

edited

dota17 Aug 21, 2020 •

edited