search

ds4dm / ecole

Extensible Combinatorial Optimization Learning Environments
https://www.ecole.ai
BSD 3-Clause "New" or "Revised" License
325 stars 68 forks source link

Extract reward before observation (C++ and Python) #212

Closed gasse closed 3 years ago

gasse commented 3 years ago

See #209

gasse commented 3 years ago

I reordered reward, observation everywhere to be consistent (even in the constructor where it does not matter).

The tests seem to pass, at least on my machine.

@AntoinePrv can you check the code for C++ (great)ness ?

gasse commented 3 years ago

All CI tests pass now. Good to go ?

AntoinePrv commented 3 years ago

There is a long commit history that does not seem to belong to this PR.

gasse commented 3 years ago

Oh no, I though I had merged that one before the release :O

@AntoinePrv that's because I have rebased dev onto master. I've rebased this PR as well, should look better now.

gasse commented 3 years ago

I guess v0.7.0 will have a very short life...

gasse commented 3 years ago

Thanks for the code review ! I'll make the changes ASAP.

gasse commented 3 years ago

I removed the auto&& universal pointer stuff, switched back to std::move() in returns, and changed the lambda for a ternary operator. I also put some stuff inside a private function, to avoid a bit of code duplication.

@dchetelat @AntoinePrv let me know how that looks to you now.

gasse commented 3 years ago

Ah it looks like some conflicting commits have not been handled correctly when I rebased dev onto master for v0.7.0 ... Thanks @dchetelat for noticing that !

dchetelat commented 3 years ago

Yeah I feel it's the third time we fix that lol. Rebasing chaos... Also I rewrote again the ternary thing, the compiler and the code check looks happy with it. All good to go for me as well!