search

opendp / opendp

The core library of differential privacy algorithms powering the OpenDP Project.
https://opendp.org
MIT License
284 stars 46 forks source link

cast int to float if needed in `py_to_c` #1509

Open mccalluc opened 3 weeks ago

mccalluc commented 3 weeks ago

Not super confident about this approach, but check_similar_scalar really seems to be the center of things: This gets called from many different points, so doing it outside this function seemed like a piecemeal solution.

This does fix test_int_data_laplace_param and I've added a lower level conversion test, but there is a change in behavior in binary search, but maybe that's ok?

Draft for now, but feedback welcome.

mccalluc commented 3 weeks ago

Error is coming from R build... probably not from this PR?

``` Error: ! error in pak subprocess Caused by error: ! `path` must exist --- Backtrace: 1. pak::lockfile_install(".github/pkg.lock") 2. pak:::remote(function(...) { … 3. err$throw(res$error) --- Subprocess backtrace: 1. base::withCallingHandlers(cli_message = function(msg) { … 2. get("lockfile_install_internal", asNamespace("pak"))(...) 3. plan$install() 4. pkgdepends::install_package_plan(plan, lib = private$library, num_workers = nw, … 5. base::withCallingHandlers({ … 6. pkgdepends:::start_task(state, task) 7. pkgdepends:::start_task_build(state, task) 8. pkgdepends:::make_build_process(path, pkg, tmp_dir, lib, vignettes, needscompilation, … 9. pkgdepends:::withr_with_libpaths(c(tmplib, lib), action = "prefix", pkgbuild::pkgbuild_… 10. base::force(code) 11. pkgbuild::pkgbuild_process$new(path, tmp_dir, binary = binary, … 12. local initialize(...) 13. pkgbuild:::rcb_init(self, private, super, path, dest_path, binary, vignettes, … 14. pkgbuild:::build_setup(path, dest_path, binary, vignettes, manual, clean_doc, … 15. base::stop("`path` must exist", call. = FALSE) 16. | base::.handleSimpleError(function (e) … 17. global h(simpleError(msg, call)) Execution halted ```

Can file an issue if we see this again. Will update and re-run CI.

raprasad commented 2 weeks ago

(leaving open until @Shoeboxam has chance to review)

Shoeboxam commented 6 days ago

The reason why it's taken me so long to review this PR is because I find the new code to be very complicated, and I feel that it could be simpler, but I don't know what to recommend off-hand. I've tinkered with it some myself, but haven't had uninterrupted time to take a good look.