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

Sketch aggregate.multi_epidist() method #270

Closed
wants to merge 6 commits into from
Closed

Conversation

Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Mar 27, 2024

This is the result from our discussion at the Epiverse - PERG development day. Key contributions were made by Anne Cori, Sangeeta Bhatia, Patrick Doohan, Christian Morgenstern, Finlay Campbell and they should probably be all listed either as function authors, package contributors, or both.

I went with an aggregate() method for this but feel free to link it to another generic if it makes more sense for you.

@joshwlambert
Copy link
Member

Thanks @Bisaloo and all other contributors. I like the idea, and the documentation and skeleton code make it clear the intention of the function. I have opened #271 to host the discussion around design decisions for this function.

@joshwlambert
Copy link
Member

Thanks to the working group that discussed this feature at the Epiverse-PERG hackathon and opened this PR. After exploring possibilities for implementing the aggregate method as sketched in the first comment of this PR I ran into a few issues that I was unable to resolve.

I have worked on this feature taking a different approach in PR #388. I propose anyone interested compare the two PRs and provide feedback. As the functionality in #388 is more complete, unless there is clear evidence that the development route taken in this PR is preferred I will close this PR and merge #388.

@joshwlambert
Copy link
Member

PR #388 introducing an aggregate() method is now merged. @Bisaloo are you happy for this PR to be closed without merging?

If the ideas sketched in this PR, not all of which are included in #388, are thought useful for the {epiparameter} package I suggest new issues are opened to propose their addition to the package.

@joshwlambert
Copy link
Member

I've agreed with @Bisaloo that this PR can now be closed without merging. Thanks to all that contributed to the PR with ideas that resulted in the new features being added in PR #388.

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.

2 participants