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

Issue #508: Aggregate data class #513

Merged
merged 50 commits into from
Feb 12, 2025
Merged

Issue #508: Aggregate data class #513

merged 50 commits into from
Feb 12, 2025

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Jan 28, 2025

Description

This PR closes #508 and #412 by adding an aggregate data class and finalising support in as_epidist_marginal_model.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@seabbs seabbs changed the title fist pass at aggregate data class Issue #508: Aggregate data class Jan 28, 2025
@seabbs seabbs force-pushed the issue508 branch 2 times, most recently from 09104ae to 2845b47 Compare February 5, 2025 13:34
Copy link
Contributor Author

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a self review this looks good to me aside from the comments I have posted and the testing gaps (see the codecov report) which need closing before this can move out of draft.

I also see that the pkgdown site is failing due to the Ebola vignette failing in the postprocessing. This works locally so is a bit of a poser but needs fixing before moving forward with this.

@epinowcast epinowcast deleted a comment from codecov bot Feb 11, 2025
@seabbs
Copy link
Contributor Author

seabbs commented Feb 11, 2025

I have now resolved all issues highlighted in a self-review so the only work that now needs doing here is to plug the remaining testing gaps for the new data format and chase down the pkgdown issue.

Copy link
Contributor Author

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another self-review. All looking good to me now. Waiting on code coverage and still need to resolve the pkgdown CI issue in the Ebola vignette

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 98.20359% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.38%. Comparing base (d7adeac) to head (240f91f).

Files with missing lines Patch % Lines
R/simulate.R 50.00% 2 Missing ⚠️
R/assert_epidist.R 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
+ Coverage   92.05%   94.38%   +2.32%     
==========================================
  Files          16       17       +1     
  Lines         743      837      +94     
==========================================
+ Hits          684      790     +106     
+ Misses         59       47      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seabbs
Copy link
Contributor Author

seabbs commented Feb 11, 2025

Unit tests are now in place covering the new functionality. Remaining small gaps are false positives from previous uncovered code.

@seabbs
Copy link
Contributor Author

seabbs commented Feb 11, 2025

Not evaluating the code for the failing plot section didn't help - it just shifted the error to the next plotting block. Usually this means that there is a problem upstream (which doesn't cause a error) or it could be a very oddly masked dependency issue. I don't currently see any issue locally.

Closing out all the final plotting blocks leads to the build succeeding.

The code inside the blocks that isn't plotting code works as expected on CI. This means the issue is localised within the plotting code (and repeated across both blocks).

@seabbs
Copy link
Contributor Author

seabbs commented Feb 12, 2025

Doing some searching and there is some suggestion this could be a cache issue (due to lazy caching being unstable with large objects). This makes some sense as the introduction of truncation here and removal of filtering the data has increased the potential size of the object quite a bit.

@seabbs seabbs marked this pull request as ready for review February 12, 2025 13:36
@seabbs
Copy link
Contributor Author

seabbs commented Feb 12, 2025

It looks like this was indeed a cache issue

Copy link
Contributor Author

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final self review. I see all checks passing and everything added covered by docs and tests. Functionality is as described in the target issue and support is present for all implemented models.

@seabbs seabbs merged commit 116659f into main Feb 12, 2025
11 checks passed
@seabbs seabbs deleted the issue508 branch February 12, 2025 13:59
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.

Add a aggregate data class
1 participant