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

Fixed comment #27

Merged
merged 6 commits into from
Oct 17, 2022
Merged

Fixed comment #27

merged 6 commits into from
Oct 17, 2022

Conversation

amorim-cjs
Copy link
Contributor

Fixed comment in line 34: xf -> dx
I suppose it is a typo

Fixed comment in line 34: xf -> dx
Testing for f=f is missing from this code, which stands for NaN checking.
Added f2003 IEEE Arithmetic that tests it explicitly.
@jacobwilliams
Copy link
Owner

jacobwilliams commented Oct 16, 2022

What is the nan check for? and what are the consequences of this check in this location?

It seems as if we wanted to check if f was nan, it would be done higher up in slsqp_wrapper rather than deep inside this routine?

amorim-cjs and others added 3 commits October 17, 2022 10:17
Check if f is NaN at convergence

Testing for f=f is missing from this code, which stands for NaN checking.
Added f2003 IEEE Arithmetic that tests it explicitly.
@amorim-cjs
Copy link
Contributor Author

At the time of writing, I was comparing with Netlib's code and just keeping the equivalence of the logical checks. Netlib's code uses NaN to empty Lagrange multipliers and boundaries, making NaN checks here and there as a sort of safety measure.
I have had more time to analyze the whole logical structure and I agree that a NaN check would be better placed in the slsqp_wraper, right after evaluating the cost function.

Hence, I've reverted the NaN check.

@jacobwilliams jacobwilliams merged commit 252ed29 into jacobwilliams:master Oct 17, 2022
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