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

add tests for blackjack edge cases #1088

Conversation

CloseChoice
Copy link
Contributor

@CloseChoice CloseChoice commented Jun 19, 2024

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Not sure if we really need these additional tests for the blackjack environment but since these cases where explicitly discussed in the linked issue I think there is no harm in adding them and making the behaviour in these cases explicit.

Fixes #1011 (issue)

Type of change

adding tests for the discussed cases.

Please delete options that are not relevant.

  • Documentation only change (no code changed)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@pseudo-rnd-thoughts
Copy link
Member

FYI, the errors are all due to NumPy 2 release, we are in the process of making a PR to fix it

@CloseChoice
Copy link
Contributor Author

CloseChoice commented Jun 20, 2024

FYI, the errors are all due to NumPy 2 release, we are in the process of making a PR to fix it

I could also restrict the numpy version to "<2" in this PR, if this is a temporary solution you consider

@CloseChoice
Copy link
Contributor Author

@pseudo-rnd-thoughts : Sorry for the delay, somehow overlooked the notification of your reaction. I updated the pyproject.toml file, we can give this a try

@CloseChoice
Copy link
Contributor Author

@pseudo-rnd-thoughts : is someone working on numpy 2 support? I took a look (at a couple of the issues) and can create draft a PR if nobody actively works on this.

@pseudo-rnd-thoughts
Copy link
Member

@CloseChoice Sorry for taking a while, we have updated the project to work with numpy 2.0

Looking at the PR, I'm a bit confused at what the PR is doing. It just adds a test meaning that there is no issue with the implementation. Therefore, do we need the test? And can we close the related issue

@CloseChoice
Copy link
Contributor Author

CloseChoice commented Jul 1, 2024

Thanks for coming back to this. I mentioned in the related issue that IMO the implementation is not a problem and there are either special rules (while the standard ones are implemented) and added tests for the standard rules that weren't covered yet, but mentioned in the issue. Also in the PR description I mentioned that we might not need this PR at all:

Not sure if we really need these additional tests for the blackjack environment but since these cases where explicitly discussed in the linked issue I think there is no harm in adding them and making the behaviour in these cases explicit.

Just thought, covering these edge cases is also some kind of documentation for further reference.

EDIT: yes, IMO the related issue can be closed.

@pseudo-rnd-thoughts
Copy link
Member

Thanks for the PR but I think it is unnecessary like you mention.
Therefore, I'm going to close this PR and related issue

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.

[Bug Report] Calculating rewards for Blackjack toy-text env
2 participants