Skip to content
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

Bug fix: Fix voltage vector size in threshold_watcher contstructor #1820

Merged

Conversation

noraabiakar
Copy link
Contributor

@noraabiakar noraabiakar commented Jan 27, 2022

Fix the following 2 issues:

  • threshold_watcher::v_prev_ is being constructed from a pointer to shared_state::voltage and a size argument not equal to the size of that vector. This isn't usually a problem because the size used is typically less than the maximum size of the voltage vector. However, it can cause problems if that assumption fails.
  • v_prev_ is copied over from shared_state::voltage before that vector is initialized to init_voltage in shared_state::reset. This can cause incorrect results at the very first application of threshold_watcher::test.

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jan 28, 2022
@noraabiakar noraabiakar marked this pull request as ready for review January 28, 2022 10:25
@bors
Copy link

bors bot commented Jan 28, 2022

try

Timed out.

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jan 31, 2022
@bors
Copy link

bors bot commented Jan 31, 2022

try

Build succeeded:

Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

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

A suggestion, rest looks fine.

{
arb_assert(n_cv_==thresholds.size());
reset();
// reset() needs to be called before this is ready for use
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Mar 23, 2022
bors bot added a commit that referenced this pull request Mar 25, 2022
@bors
Copy link

bors bot commented Mar 25, 2022

try

Build failed:

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Mar 29, 2022
@bors
Copy link

bors bot commented Mar 29, 2022

try

Build failed:

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Mar 29, 2022
@bors
Copy link

bors bot commented Mar 29, 2022

try

Build succeeded:

Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

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

👍

@thorstenhater thorstenhater merged commit 1d3ad8d into arbor-sim:master May 5, 2022
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.

2 participants