Allow supplying np_arrays or list for multi_tags positions #436

hkchekc · 2020-03-06T20:08:38Z

Also for extents.

Referring to #418, #789.

Also add some test for this constructor.

lgtm-com · 2020-03-06T20:26:30Z

This pull request introduces 1 alert when merging 41ceed4 into 5275a39 - view on LGTM.com

new alerts:

1 for Unused import

btel · 2020-03-06T20:30:47Z

Codecov Report

Merging #436 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   90.41%   90.46%   +0.04%     
==========================================
  Files          55       55              
  Lines        7087     7118      +31     
==========================================
+ Hits         6408     6439      +31     
  Misses        679      679

Impacted Files	Coverage Δ
nixio/block.py	`65.16% <ø> (+0.11%)`	⬆️
nixio/multi_tag.py	`82.32% <100.00%> (+1.48%)`	⬆️
nixio/test/test_multi_tag.py	`100.00% <100.00%> (ø)`
nixio/hdf5/h5group.py	`80.86% <0.00%> (-0.39%)`	⬇️
nixio/section.py	`80.97% <0.00%> (+0.07%)`	⬆️
nixio/property.py	`71.74% <0.00%> (+0.25%)`	⬆️

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 b19e5ad...4bc46e7. Read the comment docs.

nixio/multi_tag.py

nixio/test/test_multi_tag.py

lgtm-com · 2020-03-09T14:33:55Z

This pull request introduces 1 alert when merging 3d8d938 into e1d56ec - view on LGTM.com

new alerts:

1 for Unused import

achilleas-k · 2020-03-10T12:30:37Z

nixio/multi_tag.py

+            if name in blk.data_arrays:
+                del blk.data_arrays[name]
+            pos = blk.create_data_array(name, "{}-positions".format(self.type),
+                                        data=da)


What if da is an integer or string or some other invalid value? I think it would be confusing for the user if they were trying to create a MultiTag and they got an error about DataArray creation with invalid data. We should catch this issue as early as possible, preferably in the create_multi_tag() method in the Block class.

Also, I think this might not be the best location for the convenience functionality to be in general. Having it here makes it possible to create DataArrays by assigning values to mtag.positions. While this might sound useful, it also makes it more likely that a user tries to do something like:

mtag.positions = [10, 11] # ... later mtag.positions = [10, 11, 12] # or mtag.positions += [13]

which will lead to all sorts of errors: Duplicate names for the DataArray creation, attempting to append a list to a DataArray, etc.

I think the convenience should only be in the block.create_multi_tag() method and nowhere else, and it should be documented clearly in the arguments in the docstring. The type check for the positions argument in create_multi_tag should check for DataArray or any kind of sequence.

achilleas-k · 2020-03-10T12:38:39Z

nixio/multi_tag.py

+            ext = da
+            if not isinstance(da, DataArray):
+                blk = self._parent
+                name = "{}-extents".format(self.name)
+                if name in blk.data_arrays:
+                    del blk.data_arrays[name]
+                ext = blk.create_data_array(name,
+                                            "{}-extents".format(self.type)
+                                            , data=da)
+            self._h5group.create_link(ext, "extents")


Given my comments above about not having the convenience functionality in the positions setter, I think the same should apply to extents.
I'm thinking we could have an extents argument in the create_multi_tag() method that works the same way as positions.

In both cases (for positions and extents), I think it only really makes sense to allow the user to automatically create DataArrays for MultiTag positons and extents once per MultiTag, on creation. Otherwise, we're creating an incentive for users to create, recreate, and change the positions and extents through these creation functions and that would lead to losing track of DataArray creation, causing naming conflicts and other issues.

achilleas-k · 2020-03-11T17:23:46Z

nixio/block.py

+            positions = self.create_data_array(da_name, "{}-positions".format(type_),
+                                        data=positions)


Suggested change

positions = self.create_data_array(da_name, "{}-positions".format(type_),

data=positions)

positions = self.create_data_array(da_name, "{}-positions".format(type_),

data=positions)

achilleas-k · 2020-03-11T17:25:30Z

nixio/block.py

-        mtag = MultiTag._create_new(self, multi_tags, name, type_, positions)
+        if not isinstance(positions, DataArray):
+            da_name = "{}-positions".format(name)
+            positions = self.create_data_array(da_name, "{}-positions".format(type_),


Might be good to catch any DA creation exceptions here, specifically any exception that will be thrown if the positions data is invalid. We could then re-raise it from here with a more appropriate message that mentions the MultiTag positions array explicitly.

achilleas-k · 2020-03-11T17:26:21Z

nixio/block.py

+            extents = self.create_data_array(da_name,
+                                        "{}-extents".format(type_)
+                                            , data=extents)


Suggested change

extents = self.create_data_array(da_name,

"{}-extents".format(type_)

, data=extents)

extents = self.create_data_array(da_name,

"{}-extents".format(type_),

data=extents)

See also above comment about the DataArray creation exceptions.

achilleas-k · 2020-03-11T17:27:05Z

nixio/block.py

+            extents = self.create_data_array(da_name,
+                                        "{}-extents".format(type_)
+                                            , data=extents)
+        mtag = MultiTag._create_new(self, multi_tags, name, type_, positions, extents)


Don't change the MultiTag._create_new() method. Just add extents to the mtag after it's created.

lgtm-com · 2020-03-12T16:11:58Z

This pull request fixes 1 alert when merging 61fdde0 into ff5c813 - view on LGTM.com

fixed alerts:

1 for Unused exception object

achilleas-k

Error handling changes discussed.

lgtm-com · 2020-03-12T18:10:46Z

This pull request fixes 1 alert when merging 43019f0 into ff5c813 - view on LGTM.com

fixed alerts:

1 for Unused exception object

achilleas-k · 2020-03-13T10:34:40Z

Looks good. I'd like to see some more extensive tests. Like having invalid positions (not just None, but like a single value), valid positions but invalid extents (and check if the positions DA was deleted when the exception is caught) etc.

Let me know if you want to include them here. Otherwise rebase and I'll merge.

hkchekc · 2020-03-13T14:35:34Z

Ok, then don't merge now. I will include them here

lgtm-com · 2020-03-13T17:23:41Z

This pull request fixes 1 alert when merging 63787ad into 6d0b2f3 - view on LGTM.com

fixed alerts:

1 for Unused exception object

lgtm-com · 2020-03-13T19:03:45Z

This pull request fixes 1 alert when merging a250bd6 into 6d0b2f3 - view on LGTM.com

fixed alerts:

1 for Unused exception object

hkchekc requested a review from achilleas-k March 6, 2020 20:08

hkchekc force-pushed the conv_mt branch from 3aed837 to 41ceed4 Compare March 6, 2020 20:19

achilleas-k requested changes Mar 9, 2020

View reviewed changes

nixio/multi_tag.py Outdated Show resolved Hide resolved

nixio/multi_tag.py Outdated Show resolved Hide resolved

nixio/multi_tag.py Outdated Show resolved Hide resolved

nixio/test/test_multi_tag.py Show resolved Hide resolved

hkchekc force-pushed the conv_mt branch from 41ceed4 to 3d8d938 Compare March 9, 2020 14:27

achilleas-k requested changes Mar 10, 2020

View reviewed changes

achilleas-k requested changes Mar 11, 2020

View reviewed changes

hkchekc force-pushed the conv_mt branch from a34a6c6 to 61fdde0 Compare March 12, 2020 16:04

achilleas-k reviewed Mar 12, 2020

View reviewed changes

achilleas-k previously approved these changes Mar 13, 2020

View reviewed changes

hkchekc dismissed achilleas-k’s stale review via 9b860de March 13, 2020 17:13

hkchekc added 6 commits March 13, 2020 18:14

Allow supplying np_arrays or list for multi_tags positions

f5ff082

Fix the name and type string for auto-created positions

5c5ce26

Make multiTag convenient constructor only available on creation

c528b73

Delete positions and extents DataArrays if Multitag creationError

0fd3bc5

Add test for multitag creation error

7c8d748

Change error raising for Multitag convenient constructor

2067d34

hkchekc force-pushed the conv_mt branch from 9b860de to 63787ad Compare March 13, 2020 17:16

Add test for convenient constructor of MultiTags

a250bd6

hkchekc force-pushed the conv_mt branch from 63787ad to a250bd6 Compare March 13, 2020 18:57

achilleas-k approved these changes Mar 13, 2020

View reviewed changes

achilleas-k merged commit 38a96ab into G-Node:master Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow supplying np_arrays or list for multi_tags positions #436

Allow supplying np_arrays or list for multi_tags positions #436

hkchekc commented Mar 6, 2020

lgtm-com bot commented Mar 6, 2020

btel commented Mar 6, 2020 •

edited

Loading

lgtm-com bot commented Mar 9, 2020

achilleas-k Mar 10, 2020

achilleas-k Mar 10, 2020

achilleas-k Mar 11, 2020

achilleas-k Mar 11, 2020

achilleas-k Mar 11, 2020

achilleas-k Mar 11, 2020

lgtm-com bot commented Mar 12, 2020

achilleas-k left a comment

lgtm-com bot commented Mar 12, 2020

achilleas-k commented Mar 13, 2020

hkchekc commented Mar 13, 2020

lgtm-com bot commented Mar 13, 2020

lgtm-com bot commented Mar 13, 2020

		positions = self.create_data_array(da_name, "{}-positions".format(type_),
		data=positions)

Allow supplying np_arrays or list for multi_tags positions #436

Allow supplying np_arrays or list for multi_tags positions #436

Conversation

hkchekc commented Mar 6, 2020

lgtm-com bot commented Mar 6, 2020

btel commented Mar 6, 2020 • edited Loading

Codecov Report

lgtm-com bot commented Mar 9, 2020

achilleas-k Mar 10, 2020

Choose a reason for hiding this comment

achilleas-k Mar 10, 2020

Choose a reason for hiding this comment

achilleas-k Mar 11, 2020

Choose a reason for hiding this comment

achilleas-k Mar 11, 2020

Choose a reason for hiding this comment

achilleas-k Mar 11, 2020

Choose a reason for hiding this comment

achilleas-k Mar 11, 2020

Choose a reason for hiding this comment

lgtm-com bot commented Mar 12, 2020

achilleas-k left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 12, 2020

achilleas-k commented Mar 13, 2020

hkchekc commented Mar 13, 2020

lgtm-com bot commented Mar 13, 2020

lgtm-com bot commented Mar 13, 2020

btel commented Mar 6, 2020 •

edited

Loading