A few comments about changes, for correction in a later pull request:
1) In McSimulator::simulate(int ), it seems like a good idea to move analysis within the loop after the move. and adding a block before the loop analyze the initial state. After doing that, however, I think you do not need and should not keep the block after the end of the loop, since the final state will have been analyzed if necessary before exiting the loop. The current setup could analyze the final state twice. There should be an additional analyzer block before or after the loop, but not both.
2) In the same function, in the analysis block before the main loop, you don't need the test if (iStep % Analyzer::baseInterval == 0), since this will always return true for iStep = 0 and nonzero baseInterval.
3) In McSimulator::setup() and BdSimulator::setup, you could call functions such as McMoveManager_.needCc() and other "needs" functions directly, rather than calling a corresponding function of McSimulator or BdSimulator that simply calls this function. The indirection makes the code harder to read by forcing someone who is reading the code to first look up the wrapper function and then look up the function it calls. The wrapper "needs..." functions in the Simulator class doesn't seem to add any value, and you should consider whether it is necessary or useful, and remove it if you conclude that it is not necessary.
4) The bool that is returned by the mcMove to indicate whether it was accepted or rejected does not appear to be used or tested in the main MC loop. Since we need to test for convergences, but not for acceptance or rejection, I propose that we change the interface for McMove::move() to return true on successful convergence and false on failure to converge, and then use the same convention for BdStep::step() . This would reduce the complexity of the code required to test for convergence in both types of simulation, and would allow you to write code like
bool success;
converged = mcMoveManager_.chooseMove().move();
if (converged) {
.....
}
I think that would be preferable, but tell me if you see a problem. The inclusion of a bool to represent accept or reject seems to me to be vestigial part of the interface that seemed like a potentially useful idea but was never actually used.
5) If you find anyplace else that requires you to test for acceptance or rejection of the most recently chosen move (e.g., in any unit tests), but this isn't required in the main MC loop, you could add a protected bool isAccepted to keep track of the status of the most recently chosen move, and a function isAccepted() to return its value. If you add this, it would be set "false" by default in the constructor, set true in incrementNAccept() and set false in incrementNFail(). At the moment, however, I don't yet see anywhere that this is necessary, and would only add this if and when you found that this test is necessary somewhere.
6) If you are going to have a bool that keeps track of whether the Compressor converged during the most recent attempted move, the bool variable should be called isConverged_ and the associated function should be bool isConverged(), rather than "isConverge". A common pattern for boolean status flags and functions that return such flags is that we frequently use the word "is" followed by an adjective. Here, "converged" is being used an adjective, while "converge" is a verb.
7) In McMove, the two functions named failConverge() and successConverge(0 are badly named and involve two functions where one would do. Define a single function setIsConverged(bool isConverged) and pass it true if the compressor converges and false if it fails to converge. This was an earlier pull request, but this is as good a time as any to comment on it and ask for a change.
8) When I add a full line comment that starts with // and that describes a block of code,
I usually include a blank line before the comment and a blank line after the block of code to which it refers, to make it easy to quickly see what chunk of code such a comment refers to. You often write such comments with no added space. Please adopt the convention that uses white space in this way. Blank lines are cheap and useful - a blank line only needs to add one character to the file (a return character), and I think it makes it easier for a reader to quickly makes sense of code organization.
A few comments about changes, for correction in a later pull request:
1) In McSimulator::simulate(int ), it seems like a good idea to move analysis within the loop after the move. and adding a block before the loop analyze the initial state. After doing that, however, I think you do not need and should not keep the block after the end of the loop, since the final state will have been analyzed if necessary before exiting the loop. The current setup could analyze the final state twice. There should be an additional analyzer block before or after the loop, but not both.
2) In the same function, in the analysis block before the main loop, you don't need the test if (iStep % Analyzer::baseInterval == 0), since this will always return true for iStep = 0 and nonzero baseInterval.
3) In McSimulator::setup() and BdSimulator::setup, you could call functions such as McMoveManager_.needCc() and other "needs" functions directly, rather than calling a corresponding function of McSimulator or BdSimulator that simply calls this function. The indirection makes the code harder to read by forcing someone who is reading the code to first look up the wrapper function and then look up the function it calls. The wrapper "needs..." functions in the Simulator class doesn't seem to add any value, and you should consider whether it is necessary or useful, and remove it if you conclude that it is not necessary.
4) The bool that is returned by the mcMove to indicate whether it was accepted or rejected does not appear to be used or tested in the main MC loop. Since we need to test for convergences, but not for acceptance or rejection, I propose that we change the interface for McMove::move() to return true on successful convergence and false on failure to converge, and then use the same convention for BdStep::step() . This would reduce the complexity of the code required to test for convergence in both types of simulation, and would allow you to write code like
bool success; converged = mcMoveManager_.chooseMove().move(); if (converged) { ..... }
I think that would be preferable, but tell me if you see a problem. The inclusion of a bool to represent accept or reject seems to me to be vestigial part of the interface that seemed like a potentially useful idea but was never actually used.
5) If you find anyplace else that requires you to test for acceptance or rejection of the most recently chosen move (e.g., in any unit tests), but this isn't required in the main MC loop, you could add a protected bool isAccepted to keep track of the status of the most recently chosen move, and a function isAccepted() to return its value. If you add this, it would be set "false" by default in the constructor, set true in incrementNAccept() and set false in incrementNFail(). At the moment, however, I don't yet see anywhere that this is necessary, and would only add this if and when you found that this test is necessary somewhere.
6) If you are going to have a bool that keeps track of whether the Compressor converged during the most recent attempted move, the bool variable should be called isConverged_ and the associated function should be bool isConverged(), rather than "isConverge". A common pattern for boolean status flags and functions that return such flags is that we frequently use the word "is" followed by an adjective. Here, "converged" is being used an adjective, while "converge" is a verb.
7) In McMove, the two functions named failConverge() and successConverge(0 are badly named and involve two functions where one would do. Define a single function setIsConverged(bool isConverged) and pass it true if the compressor converges and false if it fails to converge. This was an earlier pull request, but this is as good a time as any to comment on it and ask for a change.
8) When I add a full line comment that starts with // and that describes a block of code, I usually include a blank line before the comment and a blank line after the block of code to which it refers, to make it easy to quickly see what chunk of code such a comment refers to. You often write such comments with no added space. Please adopt the convention that uses white space in this way. Blank lines are cheap and useful - a blank line only needs to add one character to the file (a return character), and I think it makes it easier for a reader to quickly makes sense of code organization.