Skip to content
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

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

Merged
merged 10 commits into from
Jul 6, 2021
Merged

Conversation

gasse
Copy link
Member

@gasse gasse commented Jun 29, 2021

See #209

@gasse gasse changed the base branch from master to dev June 29, 2021 20:34
@gasse
Copy link
Member Author

gasse commented Jun 29, 2021

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 gasse requested a review from AntoinePrv June 30, 2021 04:01
@gasse
Copy link
Member Author

gasse commented Jun 30, 2021

All CI tests pass now. Good to go ?

@AntoinePrv
Copy link
Member

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

@gasse
Copy link
Member Author

gasse commented Jul 1, 2021

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
Copy link
Member Author

gasse commented Jul 1, 2021

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

Base automatically changed from dev to master July 5, 2021 17:38
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
libecole/include/ecole/environment/environment.hpp Outdated Show resolved Hide resolved
@gasse
Copy link
Member Author

gasse commented Jul 5, 2021

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

@gasse gasse changed the title Extract reward before observation 2 Extract reward before observation (C++ environment) Jul 6, 2021
@gasse gasse changed the base branch from master to dev July 6, 2021 04:16
@gasse
Copy link
Member Author

gasse commented Jul 6, 2021

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
Copy link
Member Author

gasse commented Jul 6, 2021

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
Copy link
Contributor

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!

@gasse gasse changed the title Extract reward before observation (C++ environment) Extract reward before observation (C++ and Python) Jul 6, 2021
@gasse gasse merged commit aed579b into dev Jul 6, 2021
@gasse gasse deleted the gasse-patch-2 branch July 6, 2021 20:11
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.

3 participants