Skip to content

Fix Nix Ignoring Array Annotations #662

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jul 24, 2019
Merged

Fix Nix Ignoring Array Annotations #662

merged 11 commits into from
Jul 24, 2019

Conversation

Kleinjohann
Copy link
Contributor

The NixIO currently just ignores array annotations. I went through it with @achilleas-k and added them analogously to annotations. For writing this is straightforward, when reading I check the shape of all annotations and turn them into array annotations if it matches.

@pep8speaks
Copy link

pep8speaks commented Apr 5, 2019 •
edited
Loading

Hello @Kleinjohann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-24 09:00:27 UTC

apdavison
apdavison requested changes Apr 8, 2019
View reviewed changes
Copy link
Member

@apdavison apdavison left a comment

Choose a reason for hiding this comment

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

Looks good, but the same block of code is repeated many times, would be better to extract this into a function.

@apdavison apdavison added this to the 0.8.0 milestone Apr 8, 2019
@apdavison apdavison added the IO label Apr 8, 2019
@JuliaSprenger
Copy link
Member

Instead of guessing the type of the annotation by it's length it would be safer to introduce an additional flag for this in the nix file to be sure to recover saved array_annotations properly.

@JuliaSprenger
Copy link
Member

I had a look at it and the updated version also works for me. Depending on the result of #659, the implementation of the writing of array_annotations can be generalized (e.g. not checking for labels and durations explicitly when writing objects), but for now this is a good solution.

@JuliaSprenger
Copy link
Member

@Kleinjohann: I would like to merge this one now. Could you resolve the 4 conflicting lines?

@JuliaSprenger JuliaSprenger merged commit 6038ac1 into NeuralEnsemble:master Jul 24, 2019
This was referenced Oct 16, 2019
Closed
Merged
JuliaSprenger added a commit that referenced this pull request Oct 18, 2019
@JuliaSprenger
Merge pull request #750 from INM-6/fix/nix_array_annotations
03bef5f 
[Update to PR #662] fix duplication of array_annotations into annotations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apdavison apdavison apdavison approved these changes

Assignees

@JuliaSprenger JuliaSprenger

Labels
IO
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Kleinjohann @pep8speaks @JuliaSprenger @apdavison