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

Use abs in convergence check #72

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Use abs in convergence check #72

merged 2 commits into from
Nov 26, 2024

Conversation

bkraske
Copy link
Contributor

@bkraske bkraske commented Nov 25, 2024

Uses absolute values in the convergence check to accommodate problem formulations which converge to negative values using value iteration.

@Zinoex
Copy link
Owner

Zinoex commented Nov 25, 2024

Hi Ben, thanks for the PR! Could you clarify when this is needed? The value iteration over the properties defined in this package is monotonously increasing, so the difference should always be positive. Maybe when using the InfiniteTimeReward property?

I would like to see a test case to go with this change. If it is indeed InfiniteTimeReward, you could take a look to see if the following code fails due to the lack of abs and if so, we could comment this in and it would be the test case.

@Zinoex
Copy link
Owner

Zinoex commented Nov 26, 2024

I see the problem now - in particular, for FiniteTimeReward and InfiniteTimeReward whenever any of the immediate rewards are negative (possibly an issue for FiniteTimeSafety and InfiniteTimeSafety too). I have added appropriate tests and run the JuliaFormatter.

@Zinoex Zinoex merged commit 0fedb1a into Zinoex:main Nov 26, 2024
3 of 4 checks passed
@bkraske
Copy link
Contributor Author

bkraske commented Nov 26, 2024

Apologies for the lack of detail in my initial comment and delay in response. I believe that covers the case I had in mind.
Thank you!

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.

2 participants