overhaul with new approach #21

pearsonca · 2024-08-28T01:22:39Z

Chucks the old approach, in with the new.

pearsonca · 2024-08-28T12:45:31Z

I can deal with the linting issues, but not sure about the test-coverage - my end, the tests work (though they did some troubleshooting).

seabbs · 2024-08-28T12:57:37Z

et.hash(hash, get(".hash.salt", envir = parent.frame()))

```
├─base::set.seed(...)
```

└─base::readBin(digest(obj, algo = "xxhash32", raw = TRUE), "integer")

```
  └─base::file(con, "rb")
```

Nice! Will take a look tonight.

This is the error. Is the test CI missing a dependency?

R/zzz_distros.R

pearsonca · 2024-08-28T14:32:12Z

This is the error. Is the test CI missing a dependency?

Not sure how that gets specified - the DESCRIPTION and NAMESPACE dependencies correctly connect to digest. Whereabouts do CI package dependencies live? Presumably somewhere in the YAML configs.

seabbs · 2024-08-28T15:10:59Z

If they are in the DESCRIPTION it should pick them up. Platform specific issue maybe?

seabbs

Functionality wise this looks good. I still have big reservations about the design (i.e overwriting base functions)

seabbs · 2024-09-18T12:20:49Z

R/rrejig.R

+  formals(.FUN) <- c(formals(rFUN), alist(hash = ))
+  dispatch <- as.character(substitute(rFUN))
+  hashc <- str2lang("set.hash(hash, get('.hash.salt', envir = parent.frame()))")
+  origc <- str2lang(sprintf("%s%s%s(%s)",dispatch[2],dispatch[1],dispatch[3],toString(names(formals(rFUN)))))


bunch of linting issues here

seabbs · 2024-09-18T12:21:38Z

R/set_salt.R

+#'
+#' @export
+set.salt <- function(salt) {
+  assign(".hash.salt", salt, envir = parent.frame())


have you thought about doing this with options? I think it might be a lot more cran compliant

A little bit, but the idea is to have a scope-driven salt value - that probably means setting something in an environment.

but options can do that?

or an environment variable (options -> environment variable would be the classic approach I think)

this approach is an environment variable?

i don't think the options version let's you do this cleanly - unsetting the variable when leaving a scope seems messy?

I don't think it is its a normal variable?

This is what I mean (though I didn't mean via this nice little wrapper package): https://cran.r-project.org/web/packages/options/vignettes/envvars.html

Like the way we handle the cache location in epinowcast: https://github.com/epinowcast/epinowcast/blob/12c24bf509704c4384902408f36dff86e73fef5b/R/model-tools.R#L635

sorry, i should say: it's a normal variable, in the r-environment-scoping sense. so when r does its normal search enclosing scopes to find a token, it gets found. it also gets cleaned up etc like normal, & can exist in closer scope, while outer scope has its own (masked until that scope is returned to).

.lintr

seabbs · 2024-09-18T12:24:35Z

NAMESPACE

+export(rbinom)
+export(rcauchy)
+export(rchisq)
+export(rexp)


Do you have an issue to approach not needing to do this? It seems like quite an unsafe design and one that I don't think will fly with CRAN

I'm relatively confident this is actually what users want - minimal modification to their code to use this capability.

Great but I don't think you will be able to get it on CRAN!

🤷 give the people what they want.

that said: i agree there are tradeoffs here. my goal: enable the user to switch matching on and off, without having to change code. this approach currently only makes the refactor easy-ish. so:

make the refactor easier: provide a bit of static analysis to find invocations of the overriden functions, that don't have a hash = ... argument

make the switching (back to base) possible with an option / package environment variable

be a bit noisier when loading the package

i think adding those also helps allay some of the overloading concerns. I think there's one other, harder problem to solve: making it possible to use hashprng within another package (that does some thing that requires matching) without having that leak out of that package.

I think as suggested elsewhere if going to do this I wouldn't do it via reexporting but instead have a overload function that users can use and then use this is an option etc to overload a list of common r functions (i.e these)

you'll still have to assign to the global environment which isn't great but at least its a bit more in line with normal practice?

hmm, i don't recall that suggestion. makes some sense - so basically, users would do:

library(hashprng) hashoverload() # defaults to everything hashprng knows about ... some code ... hashunload() # turns off overloads # or hashoverload(c("unif", "binom")) # runif and rbinom only # or hashoverload(c("custom")) # wraps some rcustom that it finds in the environment

that's a little more onerous, but definitely less likely to annoy the CRAN gods.

still doesn't deal well with people having re-written their code to have hash = ... arguments, that need to be ignored if they've switched off matching.

I think for the latter I would enforce they actually use a wrapper around their dists (that can have args the dists don't have) and then they can turn hashing on and off using an option.

It means they have to rewrite their code but they can easily the turn it on and off if they are so minded?

one aside: R CMD check passes just fine.

seabbs · 2024-09-18T12:29:17Z

s live? Presumably somewhere in the YAML configs.

Yes if different to the DESCRIPTION ones

pearsonca added 5 commits August 20, 2024 11:36

WIP on hash prng update

32ae8f6

make the testthat tests works

1f85529

WIP re-factoring vignette

864de2f

working set.salt/set.hash

c49e426

remove cruft

01b1662

pearsonca requested a review from seabbs August 28, 2024 01:22

fix linting issues

07eb9c7

seabbs reviewed Aug 28, 2024

View reviewed changes

R/zzz_distros.R Show resolved Hide resolved

pearsonca added 2 commits August 28, 2024 11:52

rejig rrejig

1263ca4

pass (local) cran check

1026cfc

seabbs approved these changes Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overhaul with new approach #21

overhaul with new approach #21

pearsonca commented Aug 28, 2024

pearsonca commented Aug 28, 2024

seabbs commented Aug 28, 2024

pearsonca commented Aug 28, 2024

seabbs commented Aug 28, 2024

seabbs left a comment

seabbs Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

seabbs Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

seabbs Sep 18, 2024

seabbs Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

seabbs Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

pearsonca Sep 18, 2024

seabbs Sep 18, 2024

pearsonca Sep 18, 2024

seabbs commented Sep 18, 2024

overhaul with new approach #21

Are you sure you want to change the base?

overhaul with new approach #21

Conversation

pearsonca commented Aug 28, 2024

pearsonca commented Aug 28, 2024

seabbs commented Aug 28, 2024

pearsonca commented Aug 28, 2024

seabbs commented Aug 28, 2024

seabbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seabbs commented Sep 18, 2024