Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update success checks #29

Merged
merged 2 commits into from
May 7, 2023
Merged

update success checks #29

merged 2 commits into from
May 7, 2023

Conversation

FFroehlich
Copy link
Contributor

changes success checks heuristics a bit. notably using mean rather than first entry for consistency checks, using lower tolerances for by size checks and averaging after by size checks.

Without these changes, I rand into an abysmal number of false rejections for gradient checks with AMICI for the benchmark collection.

@FFroehlich FFroehlich requested a review from dilpath May 6, 2023 17:51
Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, why should success_by_size be determined with *tol/2, but success be determined by *tol?

How useful is this *tol/2 transformation? Does it really determine success=True/False so strongly? If so, then I guess the user should tune *tol themselves, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need tighter tolerances for the inner check otherwise the outer check will fail pretty frequently with more complex examples.

Yes ideally the user can tweak the inner tolerances as well, but all that stuff is already pretty time consuming to tweak anyways (which is really the opposite of what you want to achieve with the toolbox). Overall my impression is that it probably makes more sense to rework these heuristics rather than allowing more user tweaking.

@FFroehlich FFroehlich merged commit 24b4ab1 into main May 7, 2023
@FFroehlich FFroehlich deleted the fixes_ff branch May 7, 2023 20:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants