Skip to content

try pkg.edit.fun - #30

Merged
nmellado merged 34 commits into
mainfrom
pkg.edit.fun
Jun 16, 2026
Merged

try pkg.edit.fun#30
nmellado merged 34 commits into
mainfrom
pkg.edit.fun

Conversation

@tdhock

@tdhock tdhock commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Closes #29
Closes #22

hi @nmellado

This would be an alternative to the meta-repo idea.

The idea would be to provide a wrapper + R pkg test code which could hopefully work with any historical ponca version. (not only manually defined tags)

There are different wrapper codes in a new directory, src/wrapper-versions

  • PoncaV1x4
  • PoncaV2xalpha0
  • etc

each directory has a different version of curvatureEstimation.cpp and .h

Assume we want to test an arbitrary ponca version.
We have defined a function for editing the package, prior to installation.
In this function we try each of the wrappers, and we keep the first wrapper that compiles with the selected ponca version.

I think this would allow us to git bisect performance regressions in ponca. #28
And it seems simpler than the meta-repo.
and less maintenance.
I would have to modify atime to do checkout of the ponca submodule instead of this ponca repo, but that seems reasonable.
What do you think @nmellado ?

@nmellado

Copy link
Copy Markdown
Contributor

That would be much simpler yes.
I propose that we also add a version of the wrapper called current (or something equivalent) that could be modified by upcoming PRs if they change the API of Ponca. What do you think ?

@tdhock

tdhock commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

yes!! for current, maybe use the files in src/* (not src/wrapper-versions/current/*) ? or would we need both?
I will try to implement this approach in this branch then

@tdhock

tdhock commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

one thing I need your help with is the C++ wrapper code.
right now in this PR in interface.cpp I can call planeFit(queries, k); but not planeFit(queries, scale, meanNeiSize); (it was missing in some historical wrappers), which is why I deleted planeFit_interface() (without K) from interface.cpp. Same for asoCurvatureEstimation_interface() (without K). Can you please add those to the wrapper-versions// files?

@tdhock

tdhock commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

I added a shell script for testing each wrapper version

cd src
bash wrapper-versions/PoncaV2xalpha1.sh

that should checkout ponca version then copy the wrapper files to src/ then compile and run a test. this works for each version except PoncaV2xalpha2 which gives a floating point exception after a successful compile—is that expected?

(base) hoct2726@dinf-thock-02i:~/R/poncatime/src[pkg.edit.fun*]$ bash wrapper-versions/PoncaV2xalpha2.sh 
~/R/poncatime/src/external/ponca ~/R/poncatime/src
Previous HEAD position was 4786c3d69 Merge pull request #264 from poncateam/prepare_v.2.0.alpha1
HEAD is now at 4cffe9d86 Merge pull request #294 from poncateam/prepare_v.2.0.alpha2
~/R/poncatime/src

R version 4.5.3 (2026-03-11) -- "Reassured Reassurer"
Copyright (C) 2026 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> Rcpp::compileAttributes('..')
> 
* installing to library ‘/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5’
* installing *source* package ‘Poncatime’ ...
** this is package ‘Poncatime’ version ‘0.0.1’
** using staged installation
** libs
using C++ compiler: ‘g++ (Ubuntu 13.3.0-6ubuntu2~24.04.1) 13.3.0’
g++ -std=gnu++17 -I"/home/local/USHERBROOKE/hoct2726/lib/R/include" -DNDEBUG -Iexternal/ponca/ -Iexternal/ponca/external/eigen/ -I'/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/Rcpp/include' -I'/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/RcppEigen/include' -I/usr/local/include    -fpic  -g -O2   -c RcppExports.cpp -o RcppExports.o
g++ -std=gnu++17 -I"/home/local/USHERBROOKE/hoct2726/lib/R/include" -DNDEBUG -Iexternal/ponca/ -Iexternal/ponca/external/eigen/ -I'/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/Rcpp/include' -I'/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/RcppEigen/include' -I/usr/local/include    -fpic  -g -O2   -c curvatureEstimation.cpp -o curvatureEstimation.o
g++ -std=gnu++17 -I"/home/local/USHERBROOKE/hoct2726/lib/R/include" -DNDEBUG -Iexternal/ponca/ -Iexternal/ponca/external/eigen/ -I'/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/Rcpp/include' -I'/home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/RcppEigen/include' -I/usr/local/include    -fpic  -g -O2   -c interface.cpp -o interface.o
g++ -std=gnu++17 -shared -L/home/local/USHERBROOKE/hoct2726/lib/R/lib -L/usr/local/lib -o Poncatime.so RcppExports.o curvatureEstimation.o interface.o -L/home/local/USHERBROOKE/hoct2726/lib/R/lib -lR
installing to /home/local/USHERBROOKE/hoct2726/R/x86_64-pc-linux-gnu-library/4.5/00LOCK-poncatime/00new/Poncatime/libs
** R
** byte-compile and prepare package for lazy loading
** help
No man pages found in package  ‘Poncatime’ 
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (Poncatime)

R version 4.5.3 (2026-03-11) -- "Reassured Reassurer"
Copyright (C) 2026 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> library(testthat)
> 
> test_that("generate_data works", {
+   N_points <- 10000
+   N_queries <- 1000
+   dataScale <- 10
+   N_list <- Poncatime:::generatePointClouds_interface(N_points, N_queries, dataScale)
+   expect_equal(nrow(N_list$queries), N_queries)
+   expect_equal(nrow(N_list$points), N_points)
+   status <- Poncatime:::buildKdTree_interface(N_list$points)
+   expect_true(status)
+   retValue <- with(N_list, Poncatime:::asoCurvatureEstimationK_interface(N_list$queries, dataScale/5))
+   retValue <- with(N_list, Poncatime:::planeFitK_interface(N_list$queries, 10))
+   expect_equal(retValue$nbFit, N_queries)
+ })
wrapper-versions/PoncaV2xalpha2.sh: line 9: 2171281 Floating point exception(core dumped) R --vanilla < ../tests/testthat/test-CRAN.R

@tdhock

tdhock commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

I pushed some changes to atime (poncatime branch) and I pushed some test code to this PR, src/wrapper-versions/test.R, which works for me on ubuntu.
image

  • atime_versions() recognizes new argument checkout.path="../external/ponca" which tells it to use git checkout on that repo instead of the poncatime repo.
  • for each specified ponca version, we do git checkout and try installing wrappers until we find one that compiles.
  • each version runs the test code in the same R process

@tdhock

This comment was marked as outdated.

@tdhock

This comment was marked as resolved.

@tdhock

This comment was marked as resolved.

Toby Dylan Hocking and others added 2 commits June 11, 2026 11:11
@tdhock

This comment was marked as resolved.

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown

No obvious timing issues in HEAD=master
Comparison Plot
Generated via commit 4592300
Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 2 minutes and 39 seconds
Installing different package versions 3 minutes and 6 seconds
Running and plotting the test cases 1 minutes and 10 seconds

@nmellado

Copy link
Copy Markdown
Contributor

ok noted.
First I fix the missing functions in the existing wrappers
then I'll add this.

@nmellado

Copy link
Copy Markdown
Contributor

how should I name the folder containing the wrapper to master ?

@nmellado

Copy link
Copy Markdown
Contributor

The wrapper for v2alpha3 should also compile with the current master.
Unfortunately I miss a part of the interface (I grabbed it from a previous branch).
I need to rush, I'll fix this later on.

@nmellado

Copy link
Copy Markdown
Contributor

CI fails on something else now, the wrapper seems fine.

@tdhock

tdhock commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Current tests in this branch only have Poncatime:::asoCurvatureEstimationK_interface(N_list$queries, k=10) (with K) which did not show a difference previously. #23 (comment)

v1.0 was faster than others in that previous run for the asoCurvatureEstimation test, which uses poncatime:::asoCurvatureEstimation_interface() (no K), so I think the next step on this branch would be to add back that function to hopefully see that difference again.

update: yes we do see it.
image

@tdhock

tdhock commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

CI says https://github.com/poncateam/poncatime/actions/runs/27576962559/job/81527677041

wrapper=/tmp/RtmpXqzDFZ/file28d6b6e349b/Poncatime.v1.0/src/wrapper-versions/PoncaV1x0 compiles for v1.0
wrapper=/tmp/RtmpXqzDFZ/file28d6b6e349b/Poncatime.v1.4/src/wrapper-versions/PoncaV1x4 compiles for v1.4
wrapper= compiles for v2.0.alpha1
wrapper=/tmp/RtmpXqzDFZ/file28d6b6e349b/Poncatime.7b3f8ad3fde25a027e6452783ccee143798a71b8/src/wrapper-versions/PoncaV2xalpha3 compiles for 7b3f8ad3fde25a027e6452783ccee143798a71b8

@tdhock

tdhock commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

running git bisect on my ubuntu laptop identified poncateam/ponca@54db55b as the first commit with a slowdown in the asoCurvatureEstimation test:

54db55b7edf15c846a425584f3e9321566aa502e is the first new commit
commit 54db55b7edf15c846a425584f3e9321566aa502e
Author: Thibault Lejemble <thibault.le@live.fr>
Date:   Mon Jun 19 15:32:45 2023 +0200

    [fitting] fix bug in weight derivatives computations

 Ponca/src/Fitting/mlsSphereFitDer.hpp | 6 +++---

If it was a bug fix, probably worth keeping, even if it makes the code a bit slower.

@tdhock

tdhock commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Please merge this PR since everything seems to work now?

@nmellado

Copy link
Copy Markdown
Contributor

The test are still failing, no ?

@tdhock

tdhock commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

error is api rate limit exceeded, which I do not think we can fix.

@nmellado

Copy link
Copy Markdown
Contributor

But what the purpose of these steps of the CI ?

@nmellado
nmellado merged commit 2c338da into main Jun 16, 2026
1 of 2 checks passed
@nmellado
nmellado deleted the pkg.edit.fun branch June 16, 2026 14:20
@nmellado
nmellado restored the pkg.edit.fun branch June 16, 2026 14:20
@nmellado
nmellado deleted the pkg.edit.fun branch June 16, 2026 14:20
@tdhock

tdhock commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

we have to hit the github api for the pull request comments

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.

CXX20 support easier interface updates

2 participants