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

Restructure <epichains> and <epichains_aggregate_df> classes #79

Closed
7 tasks
jamesmbaazam opened this issue Sep 19, 2023 · 2 comments · Fixed by #107
Closed
7 tasks

Restructure <epichains> and <epichains_aggregate_df> classes #79

jamesmbaazam opened this issue Sep 19, 2023 · 2 comments · Fixed by #107
Labels
enhancement New feature or request interoperability refactoring s3-class WIP Work in progress (keep an eye out)

Comments

@jamesmbaazam
Copy link
Member

jamesmbaazam commented Sep 19, 2023

This PR is mostly adding tests, but one issue that I noticed is that <epichain> and <epichains_aggregated_df> objects can inherit from different base classes (data.frame and vector, and data.frame and list, respectively). I'm not sure whether it's a good idea to get into conditional inheritance in this way. This would probably make it difficult for future developers to easily account for what sort of object a function will return. If the data in each case (e.g. epichain_tree vs epichain_summary) is sufficiently different, perhaps it would be better to have separate classes. If you would find an overarching signature more convenient for some methods, the existing classes could be defined as abstract super-classes with sub-classes instead. Happy to discuss this further.

Originally posted by @pratikunterwegs in #71 (review)

This issue will be addressed with a PR to:

  • Remove option to return aggregations by both time and generation as it returns an <epichains_aggregate_df> that inherits from <list> while aggregations by either time or generation return a <data.frame> subclass. Associated issues: Remove conditional inheritance from aggregate method #81
  • Refactor simulate_summary() to return an <epichains_summary> class. However, this name is likely to be confused with the summary() method and needs some consideration. This will involve:
    • Setting up constructors, validators, and helpers for the <epichains_summary> class
    • Tests for the new class
  • Refactor simulate_tree() and simulate_tree_from_pop() to return an <epichains_tree> object. This will involve:
    • Setting up constructors, validators, and helpers for the <epichains_tree> class
    • Tests for the new class
@pratikunterwegs
Copy link
Collaborator

When you tackle this issue, would it be possible to define a summary.epichain() method that returns an `<epichain_summary>? That seems like it might be more user friendly.

@jamesmbaazam
Copy link
Member Author

Changes are being made in the linked PR. Reviews will be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request interoperability refactoring s3-class WIP Work in progress (keep an eye out)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants