-
Notifications
You must be signed in to change notification settings - Fork 11
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
Replicate parallel executions in forallParallelCommands
#43
Conversation
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.
Nice, I think this is heading in the right direction. The use of mask
and restore
worries me, I'd rather have it used bracket
like we used to. I also suggested a bunch of other smaller clean ups.
@stevana I addressed all your comments except the |
dfa7313
to
384bfe5
Compare
I think some of the tests that I marked as non-failing are again failing as they were before. |
Indeed, I changed |
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.
Nice work, I just noticed a few minor things, once fixed I think we can merge this!
This commit reverts 4c186f4. It also implements the change decribed in stevana#42, by which the repetition of parallel executions happens at the `forallParallelCommands` level and not in `runParallelCommands`.
daccfb5
to
7553d3e
Compare
This is an implementation of #42. In general it can be seen as a revert of #12 and #11. This also makes #40 and #41 not necessary.
I would recommend isolating only a2f27d1 to view the relevant changes, the other commit just modifies the tests.
It is however important to note that in the tests I had a bunch of troubles with
Cleanup
because it tries to test the internals of QSM, by assuming multiple repetitions in place. I'm open to discussion on how to best do this. For now as that one was using 2 repetitions I'm running the parallel machine two times.