Refactor TracePath to return a struct with relevant data#935
Refactor TracePath to return a struct with relevant data#935AndresFerCervell wants to merge 4 commits into
Conversation
tturocy
left a comment
tturocy
left a comment
There was a problem hiding this comment.
Just a minor change to be consistent in how we handle the vector of coordinates between calling the function and its return value.
|
|
||
| if (fabs(h) <= c_hmin) { | ||
| return; | ||
| return {x.back(), x, false, "Stepsize fell below minimum threshold."}; |
There was a problem hiding this comment.
I like the setup of the return struct.
I can see the benefit of having the parameter value reported separately. But actually, we should also be consistent with the calling convention of the function. At the moment TracePath is actually completely independent of where the "parameter" (TracePath itself would operate exactly the same. So there's no guarantee that calling code necessarily is using x.back() here to be the free parameter.
To be parallel with the calling convention we can simply return final_point, and then this would leave it up to the calling code how it would interpret that point.
| return RegretTerminationFunction(game, p_point, p_regret); | ||
| }, | ||
| [&callback](const Vector<double> &p_point) -> void { callback.AppendPoint(p_point); }); | ||
|
|
There was a problem hiding this comment.
Ideally we try to avoid isolated whitespace changes unless they're tied to some adjacent changes, just to keep from having spurious diffs.
| return 1; | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This is another whitespace change, but it involves removing the blank line at the end of the file. By convention files should end with a blank line. This should be caught by having the pre-commit hook installed, which is very useful for a lot of tidiness checking - see the dev docs at https://gambitproject.readthedocs.io/en/latest/developer.contributing.html#how-to-submit-a-contribution.
2 participants
Issues closed by this PR
None. Related to #492 and #876
Description of the changes in this PR
This PR adds a return structure in
TracePath(src/solvers/logit/path.[cc/h]).The struct has the following fields:
I intended to pass that error upstream by returning an empty list in
nfglogitandefglogit. However, it raises an exception in the Python API, so I have decided to remove it for now.Therefore, this PR does not solve the problem of warning the user about not having found NE, but it keeps the struct that will be of use in the future.
If it is a priority to warn the C++ users when the NE is not found, a check could be easily implemented in
logit.cc. Otherwise, I would wait untilpath.ccis abstracted to make all the changes together in Python too.Suggestions are welcome!
How to review this PR
-Review the TracePathResult struct definition in path.h and the updated return statements in path.cc.