Skip to content

Conversation

rgutzen
Copy link
Contributor

@rgutzen rgutzen commented Oct 16, 2019

Handling Issue #748
The issue arises from this part of the loading routine which adds properties which are an array_annotation to a corresponding nested dict within neo_attrs, but also adds them as a regular annotation, therefore creating a duplicate.

nixio.py line 1237-1244 (in method _nix_attr_to_neo):

                elif prop.definition == ARRAYANNOTATION:
                    if 'array_annotations' in neo_attrs:
                        neo_attrs['array_annotations'][prop.name] = values
                    else:
                        neo_attrs['array_annotations'] = {prop.name: values}
                else:
                    values = list(values)
                neo_attrs[prop.name] = values

It seems that PR #662 intended not to duplicate the array_annotations, but the duplication was then introduced with the merge commit 0a4ccfa
Please comment @Kleinjohann ?

The PR separates the properties again, to be either stored directly in neo_attrs, or in neo_attrs['array_annotations'] but not both.

Copy link
Contributor

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure I had a look at the situation with the array annotations when they were added, but with that big merge/rebase that you referenced, it looks like the logic got mangled a bit. Thanks for fixing this. See my comment below on the issue that's causing the failing test.

neo/io/nixio.py Outdated
values = values[0]
elif prop.definition == ARRAYANNOTATION:
else:
values = list(values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the reason the tests are failing. The repetition of the list() conversion here is undoing the quantity creation from earlier in the method and the unit information is lost. I think this else block is unnecessary.

@Kleinjohann
Copy link
Contributor

It seems that PR #662 intended not to duplicate the array_annotations, but the duplication was then introduced with the merge commit 0a4ccfa
Please comment @Kleinjohann ?

You are right, this is my fault, I remember something weird happening to that code block in the merge and it seems I did not resolve it correctly.

@rgutzen
Copy link
Contributor Author

rgutzen commented Oct 18, 2019

Thanks @achilleas-k , it was indeed the additional list() causing the failing of the test.

@JuliaSprenger
Copy link
Member

JuliaSprenger commented Oct 18, 2019

@rgutzen: Thanks for fixing this. Looks good to me now. Do you agree it would make sense to also add a small test based on #748? Or extend one of the existing tests to cover array annotations in this fashion?

@JuliaSprenger JuliaSprenger merged commit 03bef5f into NeuralEnsemble:master Oct 18, 2019
@rgutzen rgutzen deleted the fix/nix_array_annotations branch October 18, 2019 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants