Update operator+= logic for composite NodeCollection
#2278
Conversation
heplesser
added
I: No breaking change
nestkernel/node_collection.h
Outdated
| element_idx_ += n * step_; | ||
| // If we went past the size of the primitive, we need to adjust the element | ||
| // and primitive part indices. | ||
| size_t primitive_size = composite_collection_->parts_[ part_idx_ ].size(); | ||
| while ( element_idx_ >= primitive_size ) | ||
| { | ||
| operator++(); | ||
| element_idx_ = element_idx_ - primitive_size; | ||
| ++part_idx_; | ||
| if ( part_idx_ < composite_collection_->parts_.size() ) | ||
| { | ||
| primitive_size = composite_collection_->parts_[ part_idx_ ].size(); | ||
| } | ||
| } | ||
| // If we went past the end of the composite, we need to adjust the | ||
| // position of the iterator. | ||
| if ( composite_collection_->end_offset_ != 0 or composite_collection_->end_part_ != 0 ) | ||
| { | ||
| if ( part_idx_ >= composite_collection_->end_part_ and element_idx_ >= composite_collection_->end_offset_ ) | ||
| { | ||
| part_idx_ = composite_collection_->end_part_; | ||
| element_idx_ = composite_collection_->end_offset_; | ||
| } | ||
| } | ||
| else if ( part_idx_ >= composite_collection_->parts_.size() ) | ||
| { | ||
| auto end_of_composite = composite_collection_->end(); | ||
| part_idx_ = end_of_composite.part_idx_; | ||
| element_idx_ = end_of_composite.element_idx_; |
There was a problem hiding this comment.
Except for the different steplength in line 721, this is a duplication of the code from nc_const_iterator::operator++(). I think it would therefore be better to put this code in a separate helper function (a private function within nc_const_iterator), perhaps with the steplength as argument, and call that function from both operator++() and operator+=().
There was a problem hiding this comment.
That's true, it would make the code much nicer. Thanks! I will get to it in the next days.
|
I now put the duplicated code into a separate function "cond_update_idx". |
nestkernel/node_collection.h
Outdated
| @@ -137,6 +137,8 @@ class nc_const_iterator | |||
| size_t offset, | |||
| size_t step = 1 ); | |||
|
|
|||
| void cond_update_idx(); //!< conditionally updates element_idx and part_idx | |||
There was a problem hiding this comment.
I would prefer this to have a less cryptic name, for example composite_update_indices_() or something similar. For private class members, we end the name with a _ as a reminder that it is private. Also, for consistency, put the documentation above the declaration in a C-style comment.
nestkernel/node_collection.h
Outdated
| { | ||
| operator++(); | ||
| } | ||
| element_idx_ += n * step_; |
There was a problem hiding this comment.
This is the same as in the primitive_collection_ case, so it can be moved out before the if/else. Then the
if ( primitive_collection_ )
{...}
else
{...}can become just
if ( composite_collection_ )
{...}
nestkernel/node_collection.h
Outdated
| part_idx_ = end_of_composite.part_idx_; | ||
| element_idx_ = end_of_composite.element_idx_; | ||
| } | ||
| cond_update_idx(); |
There was a problem hiding this comment.
The element_idx_ += step_ above this line is the same in both cases here, so it can be moved out before the if/else.
|
Thanks for your feedback. I included your suggestions in this new version. |
|
@mlober There are some differences between different clang-format versions. We are enforcing formatting with clang-format 9 now, so you can try formatting node_collection.h with that and see if that fixes the problem. |
mlober
force-pushed
the
NCO_composite_operator
branch
from
8f53635
to
73de63d
Compare
stinebuu
changed the title
operator+= logic for composite NodeCollection
The operator+= of a node collection composite was changed in order to avoid the usage of the very inefficient operator++ which was previously called by operator+=. Instead of using the operator++ to loop through a NCO composite until the desired index is reached, one can now directly "jump" to this index in the new operator+=.
This change is necessary to avoid extremely large nest.connect-times when the source is a node collection composite object.