Skip to content

Validate first_step and max_accel parameters values in login_solve and related functions - #460

Closed
lusimeon wants to merge 1 commit into
gambitproject:masterfrom
lusimeon:459-validate-logit_solve-parameters
Closed

Validate first_step and max_accel parameters values in login_solve and related functions#460
lusimeon wants to merge 1 commit into
gambitproject:masterfrom
lusimeon:459-validate-logit_solve-parameters

Conversation

@lusimeon

Copy link
Copy Markdown

see #459

@lusimeon lusimeon changed the title Validate first_step and max_accel parameters values in login_solveand related functions Validate first_step and max_accel parameters values in login_solve and related functions Jun 19, 2024
@tturocy

tturocy commented Jul 18, 2024

Copy link
Copy Markdown
Member

Thank you for this. Could you also please consider adding a few tests to check these, and also to check the maxregret parameter? We acknowledge that the test suite does not yet include confirming that errors are thrown on bad arguments, but these issues are part of the process of doing that.

Also, while you are at it, the function names quoted in logit_solve_branch and logit_solve_lambda aren't correct, so if you could clean those up as well this would be super useful.

@lusimeon
lusimeon force-pushed the 459-validate-logit_solve-parameters branch from aeb1829 to 8ab08a9 Compare July 27, 2024 18:16
@lusimeon

Copy link
Copy Markdown
Author

Hi, thank you for your feedback. I've added tests to checks parameters, and correct the mispelled functions names. Seems good for you?

@tturocy

tturocy commented Sep 25, 2024

Copy link
Copy Markdown
Member

Thank you - apologies that it has taken so long to come back around to this.

I see only one minor item, which is that the inequality on the test for max_accel is inconsistent - sometimes it's weak, sometimes it's strict. A value of 1.0 is valid for the parameter, so the strict inequality < 1 would be the correct one.

@lusimeon
lusimeon force-pushed the 459-validate-logit_solve-parameters branch from 8ab08a9 to 9baebe0 Compare September 26, 2024 20:42
@tturocy

tturocy commented Oct 4, 2024

Copy link
Copy Markdown
Member

Merged into master at ad47d54 with many thanks.

@tturocy tturocy closed this Oct 4, 2024
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