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.

Sign up for GitHub

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

Jump to bottom

feat: allow custom rendering and instance generation methods for Minesweeper #85

Merged
merged 79 commits into from
Apr 3, 2023
Merged

feat: allow custom rendering and instance generation methods for Minesweeper #85

merged 79 commits into from
Apr 3, 2023

Conversation

TristanKalloniatis
Copy link
Contributor

@TristanKalloniatis TristanKalloniatis commented Mar 27, 2023 •
edited
Loading

Resolves #80

@CLAassistant
Copy link

CLAassistant commented Mar 27, 2023 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ TristanKalloniatis
❌ tristankalloniatis

tristankalloniatis seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

tristankalloniatis and others added 24 commits March 27, 2023 13:02
Some more generic fixes but more work required
f5076c2
Separate reward definitions
793af1d
Remove action history
8497f84
Cleanup
c15e722
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/env.py
8b85633 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/env_viewer.py
7bb1c40 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/generator.py
470ba66 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/utils.py
f3a7be4 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/generator.py
ee3def9 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/utils.py
8b9356c 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/utils.py
afdc1c9 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
@TristanKalloniatis @dluo96
Update jumanji/environments/logic/rubiks_cube/generator.py
0d0abdb 
Co-authored-by: Daniel <57721552+dluo96@users.noreply.github.com>
Spacing
72846b1
Merge branch 'main' into tristan/minesweeper
dba2334
More generic
14904ff
Minor cleaning
e87ddeb
Don't repeat attributes in env and generator
86f1852
Merge remote-tracking branch 'origin/main'
a2472d5
Lint
9473883
Remove minesweeper changes for separate PR
1c44c71
Simplify
8d83792
Merge remote-tracking branch 'origin/main'
5c5f8da 
# Conflicts:
#	jumanji/environments/logic/minesweeper/env.py
To sync
0bc3f33
To sync
cebb72f
dluo96
dluo96 suggested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@dluo96 dluo96 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @TristanKalloniatis! I have left a review 👍

dluo96
dluo96 previously approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@dluo96 dluo96 left a comment

Choose a reason for hiding this comment

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

LGTM! 🏎️

clement-bonnet
clement-bonnet requested changes Mar 30, 2023
View reviewed changes
Copy link
Collaborator

@clement-bonnet clement-bonnet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution!
Left a few comments regarding code suggestions, and two requests for removing some constants and removing viewer defaults. Please let me know what you think.

jumanji/environments/logic/minesweeper/conftest.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/constants.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/env.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/env.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/utils.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/utils_test.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/utils_test.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/utils_test.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/utils_test.py Outdated Show resolved Hide resolved
jumanji/environments/logic/minesweeper/viewer.py Outdated Show resolved Hide resolved
dluo96
dluo96 approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@dluo96 dluo96 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@alaterre alaterre merged commit 36ab997 into instadeepai:main Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clement-bonnet clement-bonnet clement-bonnet left review comments

@dluo96 dluo96 dluo96 approved these changes

Assignees

@TristanKalloniatis TristanKalloniatis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(minesweeper): configurable generator and viewer
5 participants
@TristanKalloniatis @CLAassistant @clement-bonnet @dluo96 @alaterre