Skip to content

Conversation

jdumouchelle
Copy link

Pull request checklist

  • I have opened an issue to discuss the proposed changes: Fix #XX.
  • I have modified/added tests to cover the new changes/features.
  • I have modified/added the documentation to cover the new changes/features.
  • I have ran the tests, checks, and code formatters.

Proposed implementation

Implementation of primal, dual, and primal dual integrals with respect to number of nodes.

From the perspective of the user, one can simply pass use_nnodes=True into the constructor of any of the integral reward functions.

@AntoinePrv AntoinePrv changed the title Node integral 2 Node difference for abscissa in Primal/Dual Integrals Oct 23, 2021
@AntoinePrv
Copy link
Member

Thanks @jdumouchelle. The code looks good to me, however I'm wondering if we should make this future-proof.
Today someone asks to use the number of nodes in the abscissa, next time it's the LP iterations...
Perhaps we could add a callback, just like we did for bound_function (and add a some aliases to make it easier to use).

Meta level

At a more abstract level, I believe Integral should in fact be more of an operator. In the same way that we can sum two reward functions, we could make it so that Integral can be computed for any two reward function (one for the abscissa and one for the ordinate).

@jdumouchelle
Copy link
Author

Yes I agree. It did come across my mind, but it will likely require a decent amount of time. Perhaps we can merge this for now so it can be used by Aaron, open an issue for a more general integral reward, and discuss it in there. After that, I will work on when I have more time.

Copy link
Member

@AntoinePrv AntoinePrv left a comment

Choose a reason for hiding this comment

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

Let's change the use_bool parameter for an enumeration, it will make it easier to add more possibilities without breaking API.
@jdumouchelle, do you want to add #198 in here or make a separate one? I feel the changes could be related.

using BoundFunction = std::function<std::tuple<Reward, Reward>(scip::Model& model)>;

ECOLE_EXPORT BoundIntegral(bool wall_ = false, const BoundFunction& bound_function_ = {});
ECOLE_EXPORT BoundIntegral(bool wall_ = false, bool use_nnodes_ = false, const BoundFunction& bound_function_ = {});
Copy link
Member

Choose a reason for hiding this comment

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

Let's change use_nnodes from a boolean to an enumeration like abscissa:

enum struct Abscissa { wall_time, n_nodes };

And

BoundIntegral(bool wall_ = false, Abscissa abscissa = Abscissa::wall_time, const BoundFunction& bound_function_ = {});

Comment on lines 64 to +65
std::vector<std::chrono::nanoseconds> times;
std::vector<SCIP_Longint> nodes;
Copy link
Member

Choose a reason for hiding this comment

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

Then what about having a single vector here like std::vector<double> abscissa instead?
Also related to #198 ?

Comment on lines -160 to +191
times.push_back(time_now(wall));
if (use_nnodes) {
nodes.push_back(get_nnodes(scip));
} else {
times.push_back(time_now(wall));
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's abstract that in a function/method, something like get_abscissa.
And elsewhere...

)");
primalintegral.def(
py::init<bool, PrimalIntegral::BoundFunction>(),
py::init<bool, bool, PrimalIntegral::BoundFunction>(),
Copy link
Member

Choose a reason for hiding this comment

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

For the enum, we have a binding function that will make it implicitly convertible from strings.

It will need to be moved inside the extension-helper library (and you might need to rebase first), and be renamed in something more sensible. Let me know if you need help with it.

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