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

Redundancy wrt applyFilters() & applySamp() #44

Open
drnickisaac opened this issue Jan 26, 2021 · 5 comments
Open

Redundancy wrt applyFilters() & applySamp() #44

drnickisaac opened this issue Jan 26, 2021 · 5 comments
Assignees

Comments

@drnickisaac
Copy link
Collaborator

applyFilters() does two jobs: sample the posterior and apply the filters, including clipping.
applySamp() does just the first.
Separating them is useful if you don't want to do any filters. It also makes the code much easier to debug. As a result there is some redundancy. This is not ideal because over time the two bits code might diverge, with unexpected consequences. A logical change would be to take the sampling part out of applyFilters(), so it reads in the output from applySamp().

@drnickisaac
Copy link
Collaborator Author

In fact this is already starting to happen. In @EllieDyer 's fork, applyFilters() has been modified to take RDS files rather than rdata (in fact the code needs to be flexible enough to read both). This change has not been extended to applySamp()

@drnickisaac
Copy link
Collaborator Author

I think that applyFilters() can be removed. You can get the same result by using applySamp() then applyClipping(). The advantage of this is that you can save time if you want multiple clipping options from the same dataset (or even no clipping).

@drnickisaac drnickisaac added this to the Sprint Feb 2021 milestone Jan 29, 2021
@drnickisaac
Copy link
Collaborator Author

This code:

sampDat <-  lapply(roster, 
                     wrappeR::applySamp,
                     parallel=TRUE, sample=TRUE)
filterDat_Gr <- rbind.fill(lapply(sampDat,
                                                 wrappeR::applyClipping,
                                                clipBy = "group")) 

Should give the same answer as:

filterDat_Gr <-  lapply(roster, 
                     wrappeR::applyFilters,
                     clipBy = "group",
                     parallel=FALSE, sample=TRUE)

Test this with a simple example. Then either remove applyFilters() or reduce to a shell that calls the other two steps.

@03rcooke
Copy link
Collaborator

03rcooke commented Feb 1, 2021

I have updated applySamp with updated applyFilters code but will test applySamp and applyClipping together before turning applyFilters into a shell

@03rcooke 03rcooke removed the sprint label Feb 1, 2021
@03rcooke
Copy link
Collaborator

I think applyFilters can just be turned into a wrapper for applySamp now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants