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

Add max rows and max columns args #96

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Add max rows and max columns args #96

merged 3 commits into from
Jun 14, 2021

Conversation

katxiao
Copy link
Contributor

@katxiao katxiao commented Jun 10, 2021

Add max rows and max columns args

@katxiao katxiao requested a review from csala June 11, 2021 06:51
@katxiao katxiao marked this pull request as ready for review June 11, 2021 06:51
csala
csala suggested changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

The approach is fine, but I made a few suggestions to clean up and improve the implementation, as well as a couple of validation checks

sdgym/datasets.py Outdated Show resolved Hide resolved
sdgym/datasets.py Show resolved Hide resolved
sdgym/datasets.py Outdated Show resolved Hide resolved
sdgym/datasets.py Outdated Show resolved Hide resolved
sdgym/datasets.py Outdated Show resolved Hide resolved
sdgym/datasets.py Outdated Show resolved Hide resolved
csala
csala approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

Looks good @katxiao !
This can be merged as soon as the checks pass

@csala csala merged commit 84a97cd into master Jun 14, 2021
@csala csala deleted the max-rows-cols branch June 14, 2021 14:35
@katxiao katxiao added this to the 0.4.0 milestone Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csala csala csala approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@katxiao @csala