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

Expand model description. #373

Merged
merged 16 commits into from Apr 27, 2023
Merged

Expand model description. #373

merged 16 commits into from Apr 27, 2023

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Mar 23, 2023

An attempt to make the model description self-contained, i.e. allow others to re-implement the full model (almost) completely.

@sbfnk sbfnk requested a review from seabbs March 23, 2023 18:07
@seabbs seabbs added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 23, 2023
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.

Looks good to me. I added a few comments but no real errors or issues.

@sbfnk
Copy link
Contributor Author

sbfnk commented Apr 13, 2023

This now also has the backcalculation model so would benefit from another round of review.

@sbfnk sbfnk requested a review from seabbs April 13, 2023 19:30
@seabbs
Copy link
Contributor

seabbs commented Apr 13, 2023

This now also has the backcalculation model so would benefit from another round of review.

Will do tomorrow.

Base automatically changed from develop to main April 27, 2023 08:36
@seabbs
Copy link
Contributor

seabbs commented Apr 27, 2023

This is a nice way of describing this.

@sbfnk
Copy link
Contributor Author

sbfnk commented Apr 27, 2023

This is a nice way of describing this.

Nothing to see sadly (even before I accidentally edited your comment to remove the invisible screenshot).

@seabbs
Copy link
Contributor

seabbs commented Apr 27, 2023

The change to have the log difference on the LHS.

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.

This is nice. I made some changes and pulled out the GP details section into its own vignette (as may be shared between models etc). The biggest change is changing the name of the non-parametric model -> non-mechanistic because non-parametric models are a dream of AI bros.

I've pushed 1.3.5 into its own branch in case it gets bounced by CRAN (so we can update main). I'll leave this with you to merge as you wish/once you've made any changes you would like to.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 555debd is merged into main:

  •   :ballot_box_with_check:default: 50.1s -> 50.7s [-17.16%, +19.51%]
  •   :ballot_box_with_check:no_delays: 37.4s -> 37.3s [-11.67%, +10.91%]
  •   :ballot_box_with_check:random_walk: 12.8s -> 13.1s [-9.5%, +14.26%]
  •   :ballot_box_with_check:stationary: 30.8s -> 30.1s [-6.31%, +1.87%]
  •   :ballot_box_with_check:uncertain: 47s -> 47.1s [-15.25%, +15.64%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs
Copy link
Contributor

seabbs commented Apr 27, 2023

(going to merge and we can iterate)

@seabbs seabbs added this pull request to the merge queue Apr 27, 2023
Merged via the queue into main with commit 0c04c4a Apr 27, 2023
11 checks passed
@seabbs seabbs deleted the update-model-description branch April 27, 2023 14:24
sbfnk pushed a commit that referenced this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants