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.

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

Use vector of integer for per-thread bool indicators (backport) #1455

Merged
merged 85 commits into from
May 25, 2020
Merged

Use vector of integer for per-thread bool indicators (backport) #1455

merged 85 commits into from
May 25, 2020

Conversation

jasperalbers
Copy link
Contributor

cherry-picked commits from pull request #1442, resolved a minor merge conflict

@heplesser heplesser requested review from heplesser and suku248 March 6, 2020 14:56
@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) P: Blocked Work on this can not continue, see comments for the particular reason S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation labels Mar 6, 2020
@heplesser
Copy link
Contributor

heplesser commented Mar 6, 2020 •
edited
Loading

@jasperalbers Thank you for preparing v2.20.1.

In addition to #1442, please also integrate #1416, #1421, #1422, #1427, #1428, #1430, which all corrected weaknesses in the code which we discovered while solving #1394.

And integrate #1458, which solves #1449.

@heplesser heplesser added ZP: PR Created DO NOT USE THIS LABEL and removed P: Blocked Work on this can not continue, see comments for the particular reason labels Mar 9, 2020
@heplesser
Copy link
Contributor

@jasperalbers All necessary PRs have now been merged into master, so you can proceed with cherry-picking into this branch.

jakobj added 3 commits March 13, 2020 10:25
@jakobj @jasperalbers
Fix misuse of outdated variable that led to an impossible condition
0f2d1e1 
This removes a local counter used as an exit condition for a while
true loop that was never increased. It is replaced by using the
correct functions of send_buffer_positions.
@jakobj @jasperalbers
Refactor SendBufferPosition, fail on query with rank outside of range
5516e8a
@jakobj @jasperalbers
Use proper delete[] and atomic read in CompletedChecker
1b29655
@jasperalbers
Copy link
Contributor Author

@heplesser I was advised by @terhorstd to create a separate pull request onto nest-2.20.1 for each of the PRs that you mentioned. Here is a complete list:

for #1416 : new PR #1462
for #1421 : new PR #1463
for #1428 : new PR #1464
for #1458 : new PR #1465

Note: @terhorstd suggested to exclude #1422, #1427 and #1430 as they are not bugfixes but rather developments not necessary for a NEST version that won't be maintained in the future.

Additional note: PR #1458 addresses changes in two files that are not present in NEST 2.20, namely models/music_rate_in_proxy.h and models/music_rate_in_proxy.cpp. I assume that they are supposed to be added, which I did in #1465.

@heplesser heplesser added this to the NEST 2.20.1 milestone Mar 13, 2020
@heplesser
Copy link
Contributor

@jasperalbers Ok with the independent PRs, I just hope we don't get problems due to dependencies between the different fixes. I have merge two of the new PRs and will merge the third once tests have gone through (timeout on Travis).

@terhorstd @jasperalbers I would feel much more comfortable also to integrate #1422, #1427 and #1430, since they all were part of one development stream that led to the fix of #1394.

@jasperalbers jasperalbers mentioned this pull request Apr 24, 2020
Merged
@jasperalbers jasperalbers changed the title v2.20.1 Backport of #1442 to 2.20.1 Apr 24, 2020
@jasperalbers
Copy link
Contributor Author

#1502 can be ticked off the list (which isn't strictly necessary anymore, all corresponding PRs can be found under the milestone 2.20.1), and this PR itself needs reviews and merging.
I renamed the PR to make it consistent with all the other backporting PRs.

@heplesser
Copy link
Contributor

@jasperalbers Please also integrate #1566, it is just a single line.

@jasperalbers
Copy link
Contributor Author

@heplesser done in #1572.

@heplesser
Copy link
Contributor

@heplesser done in #1572.

@jasperalbers Thanks! Then we just need to wrap up the review of #1472 (@terhorstd), resolve the merge conflicts and be ready for the final review of this PR?

@terhorstd
Merge pull request nest#1472 from steffengraber/doc-final-check
3be9ac2 
Final updates in documentation and the debian package for v2.20.1
@heplesser
Copy link
Contributor

@jasperalbers I think we have all individual pieces for 2.20.1 in place now. Could you check and solve the merge conflicts? Then we can review and hopefully release soon.

@jasperalbers
Copy link
Contributor Author

@heplesser great! I also don't see any loose ends. I rebased my heisenbug-fix-branch to the current nest-2.20.1 and now all conflicts should be solved.

heplesser
heplesser requested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@jasperalbers Looks good, just one tiny comment concerning the links to the live media.

doc/download.rst Outdated
* `NEST live media 2.16.0 <https://nest-simulator.org/downloads/gplreleases/lubuntu-18.04_nest-2.16.0.ova>`_

`Checksum 2.16.0 <https://nest-simulator.org/downloads/gplreleases/lubuntu-18.04_nest-2.16.0.ova.sha512sum>`_
* `NEST Live Media 2.20.0 <https://nest-simulator.org/downloads/gplreleases/lubuntu-18.04_nest-2.20.0.ova>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

The links to 2.20.0 should also move down to "Older versions".

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved 2.20 to older versions.

suku248
suku248 reviewed May 25, 2020
View reviewed changes
Copy link
Contributor

@suku248 suku248 left a comment

Choose a reason for hiding this comment

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

@jasperalbers thank you!
Looks good to me. However, this PR includes many different commits. Are there any lines of code that caused merge conflicts that you found difficult to solve? I could take a look at them.

@jasperalbers
Copy link
Contributor Author

@suku248 nothing particular comes to my mind. Notice that only your 6 commits are the core of this PR, the rest is here just from rebasing to the current 2.20.1 branch and are already reviewed!

suku248
suku248 approved these changes May 25, 2020
View reviewed changes
Copy link
Contributor

@suku248 suku248 left a comment

Choose a reason for hiding this comment

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

Alrighty!

@suku248 suku248 merged commit ac22785 into nest:nest-2.20.1 May 25, 2020
@jougs jougs changed the title Backport of #1442 to 2.20.1 Use vector of integer for per-thread bool indicators (backport) Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terhorstd terhorstd terhorstd left review comments

@heplesser heplesser heplesser approved these changes

@suku248 suku248 suku248 approved these changes

Assignees

@jasperalbers jasperalbers

Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation
Projects
None yet
Milestone
NEST 2.20.1
Development

Successfully merging this pull request may close these issues.
10 participants
@jasperalbers @heplesser @suku248 @terhorstd @jakobj @steffengraber @sarakonradi @hakonsbm @stinebuu @gtrensch