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

Update nloptr c #169

Merged
merged 25 commits into from
Mar 10, 2025
Merged

Update nloptr c #169

merged 25 commits into from
Mar 10, 2025

Conversation

aadler
Copy link
Contributor

@aadler aadler commented Jul 2, 2024

While reading through WRE, I noticed that Rdefines.h is no longer maintained, and that is what @jyypma used in nloptr.c, so I figured we should bring nlopt.c more in line with the API. While going through the file I made some other changes (see the commit log for exact details). They include:

  • Import Rinternals.h instead of Rdefines.h and manually define the necessary coercion shortcuts.
  • Bugfix: ranseed was processed through the AS_INTEGER macro, which casts to int although nlopt_srand requires an unsigned long. It is now coerced to a double and then cast to the unsigned long retaining the full range of random seeds.
  • Convert loop variables to size_t where appropriate and declare them inside the for loops for safety.
  • Use more R-centric accessor functions where appropriate (e.g. asReal, asInteger).
  • Convert the huge nested if-else ladder in "find algorithm" to a switch statement based on an algorithm lookup table.
    • This should be more efficient as strcmp is now only called once. The table has the algorithms in lexicographical order, so bsearch can be used to find the proper case. If a new algorithm is added, it should be added in alphabetical order and the table updated.
  • Some minor efficiency in the gradient nested loops by calculating values which will be constants in the inner loop prior to entering the loop instead of each time through the inner loop.
  • Creating pointer variables outside of loops instead of continually calling the REAL or INTEGER macros inside the loops. See the section titled Accessing vector data by Hadley.
  • Minor cleanup of DESCRIPTION (Biarch is no longer needed, but NeedsCompilation and UseLTO should be set).
  • Declare and initialize variables on the same line where appropriate.
  • Use unsigned integers/size_t in structures where numbers < 0 make no sense.
  • Copy-edit comments.
  • block out some C code in nocov which cannot be tested either because they depend on NLopt functions which are not exposed or because they are completely guarded by is.nloptr.

aadler added 22 commits July 2, 2024 10:41
…e efficient to define a pointer outside the loop and use that instead.
…table and switch statement. Should be more efficient (only 1 strcmp call) and more elegant.
…) needs a long. Instead, pass as a real and cast to long in C.
@aadler
Copy link
Contributor Author

aadler commented Jul 3, 2024

Correction. Strcmp can be called more than once, but since it’s a bisection search on an ordered list, it should be called fewer times than the if-else ladder.

@aadler
Copy link
Contributor Author

aadler commented Jul 23, 2024

Team, any thoughts?

@aadler
Copy link
Contributor Author

aadler commented Nov 24, 2024

@astamm Any objections?

@aadler
Copy link
Contributor Author

aadler commented Mar 3, 2025

Once the pull requests fixing the packages are completed, this PR may need to be retested.

@astamm
Copy link
Owner

astamm commented Mar 4, 2025

I merged #176. I will run reverse dependency checks today. Then, I'll merge the fixes in this PR and see if it breaks or not.

@astamm
Copy link
Owner

astamm commented Mar 9, 2025

Nice changes, thank you @aadler! I merged the master and slightly edited nloptr.c. It seems not to break anything now. Can you test quickly on your side? My plan is to submit to CRAN today or tomorrow at the latest. Thanks again.

@aadler
Copy link
Contributor Author

aadler commented Mar 9, 2025

Unfortunately I can’t right now. I just built a new PC and am having trouble building R 😞

@astamm astamm merged commit 5a3303b into astamm:master Mar 10, 2025
9 checks passed
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