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.

Sign up for GitHub

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

Jump to bottom

Issue 163: Create epidist_diagnostics #175

Merged
merged 12 commits into from
Jul 19, 2024
Merged

Issue 163: Create epidist_diagnostics #175

merged 12 commits into from
Jul 19, 2024

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Jul 17, 2024

Description

This draft PR will close issue #163.

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.

@athowes athowes changed the title Issue 163: Create epidist_diagnostics Issue 163: Create epidist_diagnostics Jul 17, 2024
@athowes
Copy link
Collaborator Author

athowes commented Jul 17, 2024 •
edited
Loading

This is incomplete but I'd be interested in feedback before finishing off. In my mind remaining things are:

  • Add time taken
  • Complete unit tests
  • Double check that no useful diagnostics really are available for methods outside "sampling"
  • Investigate why epidist_diagnostics seems to take a reasonably long time (can it be quicker?) -- decided not worth it to do this
  • Document functions
  • Not really happy with the placement of functions into files / families but I think it's a wider issue and should be rethought more broadly (perhaps mention this in issue about it How to document S3 functions #162)
  • Replace use of custom code in expect_convergence with use of epidist_diagnostics
  • See if this can be simplified without using S3

@athowes athowes requested a review from seabbs July 17, 2024 14:37
@athowes athowes force-pushed the sample-model-transfer branch from bb61daa to 18c6e53 Compare July 18, 2024 09:54
@seabbs
Copy link
Contributor

seabbs commented Jul 18, 2024

Is this stil la draft or ready for review?

@athowes
Copy link
Collaborator Author

athowes commented Jul 18, 2024

Just need to fix tests then will be ready for review. Will message you or tag

@athowes athowes marked this pull request as ready for review July 19, 2024 09:59
@athowes
Copy link
Collaborator Author

athowes commented Jul 19, 2024

Ready! @seabbs

seabbs
seabbs approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@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.

Nice. LGTM. The workflow (fit -> diagnostics) is much better.

@seabbs seabbs merged commit 55ab8bf into main Jul 19, 2024
7 checks passed
@seabbs seabbs deleted the sample-model-transfer branch July 19, 2024 10:24
@athowes athowes mentioned this pull request Jul 22, 2024
Closed
seabbs pushed a commit that referenced this pull request Jan 10, 2025
@athowes
Issue 163: Create epidist_diagnostics (#175)
2a0ff03 
* Remove sample_model

* Start to write epidist_diagnostics

* Rebase merged PRs

* Don't need this to be S3

* Finish removal of S3 and improve tests

* Replace custom code with diagnostics function

* Lint and documentation fixes

* Add documentation to epidist_diagnostics

* Lint and add diagnostics as section

* Add more unit tests for diagnostics

* expect_numeric doesn't exist

* Make tests softer

Former-commit-id: b8ca220f6c7c53a209f818c731d29b40b25c2298 [formerly e1e900b581ace52c1d7da304774f1623dc1c0a59]
Former-commit-id: 2e5b3af9e2256b55fb2785436eaf15b28bcb5790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seabbs seabbs seabbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@athowes @seabbs