-
Notifications
You must be signed in to change notification settings - Fork 531
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
Fix some todos #1646
base: master
Are you sure you want to change the base?
Fix some todos #1646
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
h5py/h5p.pyx
Outdated
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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when len == 0
? What's returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should do something when an error is detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aragilar should we tackle it like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The order tracking looks good to me (though the 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
h5py/_hl/dataset.py
Outdated
@property | ||
@with_phil | ||
def track_order(self): | ||
""" Whether dataset creation order are tracked (T/F)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" Whether dataset creation order are tracked (T/F)""" | |
""" Whether attribute creation order is tracked (T/F)""" |
h5py/_hl/dataset.py
Outdated
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
h5py/_hl/dataset.py
Outdated
def track_times(self): | ||
""" Whether times associated with an object are tracked (T/F)""" | ||
dcpl = self._dcpl | ||
return dcpl.get_obj_track_times() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't the first time exposing get_obj_track_times
in the high level API, it comes from here
Line 263 in 15ed70a
kwupdate.setdefault('track_times', dcpl.get_obj_track_times()) |
h5py/_hl/group.py
Outdated
else: | ||
order = True | ||
if not order == track_order: | ||
raise TypeError("tack_order do not match (existing %s vs new %s)" % (order, track_order)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I will leave it as-is, just remove this kind of TODO.
h5py/h5p.pyx
Outdated
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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rerun to pass static-check. |
Fix some legacy TODOs.
CC: @takluyver @aragilar