-
Notifications
You must be signed in to change notification settings - Fork 3
fix: decode numpy arrays; feature: topostats versions #177
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
Conversation
I found whilst working through TopoStats refactoring that the inclusion of the `config` as an attribute to `TopoStats`
objects resulted in some of the values, which are lists of strings, being converted to Numpy arrays.
These could not be decoded direction and `item[()].decode("utf-8")` failed with an `AttributeError` stating that `numpy.ndarray
does not have attribute decode`.
The solution proposed here is to capture this error and if `item[()]` is an instance of `np.ndarray` to iterate over the
list decoding each item in turn. The typing is explicitly ignored because we want it as a list rather than a
dictionary.
Test included, and whilst it passes seems a bit light but does mirror the scenario encountered (had to use `group_path`
as the higher level of nesting).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #177 +/- ##
==========================================
+ Coverage 74.62% 79.46% +4.83%
==========================================
Files 8 12 +4
Lines 607 935 +328
==========================================
+ Hits 453 743 +290
- Misses 154 192 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9197463 to
0996cdc
Compare
Restructuring of TopoStats to define classes means the `TopoStats` object now holds the `topostats_version` rather than `topostats_file_version`. This commit allows both to be handled and switches to using [`packging.version`](https://packaging.pypa.io/en/stable/version.html) to do so which ensures a more consistent approach to comparing version numbers. Yet to write a test for working with newer `.topostats` where `topostats_version >= 2.3.2` as work is still on-going but parameterised test is in place for when work is complete.
0996cdc to
0b1c527
Compare
|
Discovered a minor problem with this as I'd used Ready for review and will need merging and releasing around the same time as |
|
We could do with merging this at least as the TopoStats Refactor Pull Request (see 1284) requires this fix. I can work around it for now by installing |
SylviaWhittle
left a comment
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.
Sorry for the delay. I don't have time to try the code out, but recognise this needs to get merged.
Decode numpy arrays of strings
I found whilst working through TopoStats refactoring that the inclusion of the
configas an attribute toTopoStatsobjects resulted in some of the values, which are lists of strings, being converted to Numpy arrays (technically everything is converted to Numpy arrays as HDF5 can't handled "lists" of mixed types and so they are coerced to Numpy arrays with a singledtype).However, these could not be decoded directly and
item[()].decode("utf-8")failed with anAttributeErrorstating thatnumpy.ndarray does not have attribute decode.The solution proposed here is to capture this error and if
item[()]is an instance ofnp.ndarrayto iterate over the list decoding each item in turn. The typing is explicitly ignored because we want it as a list rather than a dictionary.Test included, and whilst it passes seems a bit light but does mirror the scenario encountered (had to use
group_pathas the higher level of nesting).Handle newer topostats file versions
Restructuring of TopoStats to define classes means the
TopoStatsobject now holds thetopostats_versionrather thantopostats_file_version.This commit allows both to be handled and switches to using
packging.versionto do so which ensures a more consistent approach to comparing version numbers.Yet to write a test for working with newer
.topostatswheretopostats_version >= 2.3.2as work is still on-going but parameterised test is in place for when work is complete.