Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Move ElectrodesTable creation #161
base: main
Are you sure you want to change the base?
Move ElectrodesTable creation #161
Changes from 8 commits
742e31e
6b4b62f
39ae551
3128d22
e6225e5
830aea1
fe20dd2
6146a72
ffdcde3
4c4ce2d
410da4a
6455265
4c71030
ac123bd
3c0b98e
e3ef713
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
getStorageObjectShape
.ReadDataWrapper
to make this new function convenient to use on readThere 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.
That makes sense, I will add functionality for attributes and use
shape
to refer to sizes along all dimensionsThere 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.
Maybe
Types.h
is a better place for this. Alternatively, it seems thatm_emptyContainerIndexes
is only used to set default arguments of functions, so we could be explicit and just use "std::vector()" as default argument, i.e., use: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.
If I remember correctly, I think I set this up this way because you cannot pass a temporary empty initializer list as a default argument to a non constant reference parameter. Creating the empty vector in advance was a solution for that. Another potential option is to change the parameter to an rvalue reference, but then I think there might be additional complications in terms of transferring ownership.
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.
If the ElectrodeGroup data must be written, should this function then call
m_io->flush()
here first?