hugheylab / phers

https://phers.hugheylab.org
0 stars 0 forks source link

genotype association function #13

Closed arefnel closed 2 years ago

jakejh commented 2 years ago

Indeed, we don't want that warning. I recall we already implemented a solution for this issue in one of our other packages that allows us to avoid the ..variable syntax.

ehoutland commented 2 years ago

https://github.com/hugheylab/phers/blob/26928bda4d4b4683493b82abe1327cdc9c69ebce/R/phers.R#L66

Is there a difference between returning weights[] and weights? I tried changing it and got the same results.

Same question here: https://github.com/hugheylab/phers/blob/26928bda4d4b4683493b82abe1327cdc9c69ebce/R/phers.R#L106

arefnel commented 2 years ago

https://github.com/hugheylab/phers/blob/26928bda4d4b4683493b82abe1327cdc9c69ebce/R/phers.R#L66

Is there a difference between returning weights[] and weights? I tried changing it and got the same results.

Same question here:

https://github.com/hugheylab/phers/blob/26928bda4d4b4683493b82abe1327cdc9c69ebce/R/phers.R#L106

Yes when I used weights the function would return an invisible copy of the weights table. Apparently, this is an issue with data tables that happens when you use := in the line previous to return()

JSchoenbachler commented 2 years ago

@jakejh as a lab standard moving forward, if we are returning a data.table object and run into the invisible return issue, do we want to keep taking the return(dt[]) approach? If it works, we could instead do return(copy(dt)).

I say "if it works", because I actually can't replicate this issue, is it platform specific?

EDIT: Of course as soon as I posted this comment I was able to replicate it. the return(copy(dt)) works as well. I think I'd just like to know which one we settle on because I might add a little blurb in the technical docs since this has come up before.

jakejh commented 2 years ago

No, I don't think we want to use copy. You tell me why.

JSchoenbachler commented 2 years ago

It's less efficient. Instead of using the already referenced and created values, this would copy those values unnecessarily before returning. Got it!

jakejh commented 2 years ago

@ehoutland Are you done with your review? Let's have a standard where the reviewer says when each round is complete.

ehoutland commented 2 years ago

Yes, I'm done.

jakejh commented 2 years ago

@arefnel Can you simplify all the \{describe\item{\code{...}}} stuff to the markdown syntax as we use in limorhyde2?

https://github.com/hugheylab/limorhyde2/blob/master/R/get_stats.R#L20-L30

jakejh commented 2 years ago

@arefnel Can you mark conversations as resolved and then Elliot and I will go through once more?