#8 [2/4]: Model fit helpers #18

zsusswein · 2024-06-12T01:27:53Z

Important

This PR a trial for using a stacked PR workflow. Do not merge.

This PR builds on #14 to add two helper functions. Both are necessary prerequisites for the actual model fitting. They suggest a basis dimension for use in the model and construct the actual model formula.

The model formula constructor is meant to be easily extensible in the future to add hierarchy, day of week effects, and more. That isn't in scope for this PR, but if there are extensibility concerns it would be good to flag those now.

It would also be good to get feedback on some of the vibes-based decisions in here, The two big ones are the choice of basis dimension suggestion and the three-week-per-penalty-dimension adaptive smooth. The smoothing basis dimension heuristic a guess and I try to be honest in the documentation about that. I do think it's a decent guess, but if there are real concerns that it's not great I can change things up.

Likewise with the choice of three weeks per additional penalty basis. I think it's an approximately reasonable choice, but there will be tradeoffs. For each, I think it's important that we don't make the perfect the enemy of the good. Better to make some reasonable choices here to get to a v0.1 with a full package so we can do some more robust testing.

Also tweaked some checker functions to make them more multipurpose

zsusswein · 2024-06-12T12:25:37Z

Opening against main to get codecov on CI, but will target #14 as the base for review.

codecov · 2024-06-12T12:27:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5ee9a2c) to head (fda5165).

Additional details and impacted files

@@            Coverage Diff             @@
##              main       #18    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            3         5     +2     
  Lines           87       208   +121     
==========================================
+ Hits            87       208   +121

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

seabbs

This all looks reasonable and I like the majority of the docs a lot.

I'd suggest another pas on the overall docs. I was also a little confused as to why the smoothness penalty was hard coded vs also being treated as user specified?

I think ideally uses should be able to choose between adaptive or non-adaptive and I would just throw a warning if a adaptive spline is used with some small n.

Before merging this PR I would look to see an issue investigating the smoothness defaults for the adaptive spline.

R/RtGam.R

R/formula.R

zsusswein · 2024-06-12T15:12:33Z

I was also a little confused as to why the smoothness penalty was hard coded vs also being treated as user specified?

This is good feedback. I think for the current functionality, the process is a little convoluted. But, this package is being developed with an eye to implementing day of week effects, partial pooling, etc. When implemented, we'll need to partition the degrees of freedom between these different components -- this process is how I propose to do that. We allow a user-specified global degrees of freedom and handle the partitioning between components internally.

Perhaps we should allow the user to specify individual components' degrees of freedom, but I think it's important we provide a decent first pass there by default.

I think ideally uses should be able to choose between adaptive or non-adaptive and I would just throw a warning if a adaptive spline is used with some small n.

This is good feedback. I'll implement here.

athowes

Just getting up to speed with what's going on here so likely not very helpful review comments but in case.

R/RtGam.R

athowes · 2024-06-12T16:47:25Z

I think ideally uses should be able to choose between adaptive or non-adaptive and I would just throw a warning if a adaptive spline is used with some small n.

I agree that the user should be able to choose between adaptive and non-adaptive. I guess that's not something we have implemented yet (as in so far there is just the one k, and the model isn't fit.)

Just looking, for the group option -- do we want to explicitly link that to "geographic". Is it not also possible to be other possible variables?

When implemented, we'll need to partition the degrees of freedom between these different components -- this process is how I propose to do that. We allow a user-specified global degrees of freedom and handle the partitioning between components internally.

Feels like we might as well have the option for the user to specify the partition too, just the same default argument set-up as we have for the global (i.e. we have a heuristic function, and you can override it if you really want.)

@seabbs

And instead point to `dimensionality_heuristic()` as the sole source of guidance per @seabbs

To prepare also a user-specific penality basis dimension with associated heuristic default

This change involves a refactor to the smooth basis dimension selector name, updates to function signatures, and changes to unit tests. More usefully, I consolidated the penalty dimension documentation into the penalty basis dim function, tried to match to doc style of the smooth basis dim selector, and reduced a fair amount of duplication.

zsusswein · 2024-06-24T13:53:45Z

I'd suggest another pas on the overall docs.

Did some rewriting and condensing

I agree that the user should be able to choose between adaptive and non-adaptive. I guess that's not something we have implemented yet (as in so far there is just the one k, and the model isn't fit.)

I think ideally uses should be able to choose between adaptive or non-adaptive

Done

I would just throw a warning if a adaptive spline is used with some small n.

Will address in the next PR on the model fitting

Before merging this PR I would look to see an issue investigating the smoothness defaults for the adaptive spline.

#19

for the group option -- do we want to explicitly link that to "geographic". Is it not also possible to be other possible variables?

Good catch. Modified it to be more general.

Feels like we might as well have the option for the user to specify the partition too, just the same default argument set-up as we have for the global (i.e. we have a heuristic function, and you can override it if you really want.)

I want to keep this out of scope here, but will revisit once there's some actual partitioning happening (i.e., day of week effect and/or hierarchical model)

R/formula.R

seabbs

Overall looks good to me. A very few very minor additional comments.

Allow optional switching between gam() and bam(), both for functionality now and to illustrate how one might extend to different backends in the future.

It makes documentation slightly more straightforward

Allowing future extensions.

@seabbs

As suggested by @seabbs. This allows ... to be evaluated and handles dispatch.

Has basic info but still needs model fit diagnostics

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>

By pointing the public function to the slot with the stored diagnostic list. Re-order `RtGam()` to use the public function for diagnostic reporting.

zsusswein added 3 commits June 10, 2024 12:41

Add k argument with documentation

99272a3

Default basis dimension selector and documentation

ec173d6

Implement and test setting k

80a451d

Also tweaked some checker functions to make them more multipurpose

zsusswein force-pushed the 08-model-fit-helpers branch from 8de5ecf to 01ecfda Compare June 12, 2024 01:39

Model formula helper and documentation

0b5cd8c

zsusswein force-pushed the 08-model-fit-helpers branch from 01ecfda to 0b5cd8c Compare June 12, 2024 12:20

zsusswein marked this pull request as ready for review June 12, 2024 12:25

zsusswein requested review from kgostic and seabbs as code owners June 12, 2024 12:25

zsusswein changed the base branch from main to 08-class-constructor June 12, 2024 12:27

zsusswein requested a review from athowes June 12, 2024 12:43

zsusswein added the v0.1.0 label Jun 12, 2024

seabbs reviewed Jun 12, 2024

View reviewed changes

athowes approved these changes Jun 12, 2024

View reviewed changes

R/RtGam.R Outdated Show resolved Hide resolved

R/RtGam.R Outdated Show resolved Hide resolved

R/RtGam.R Show resolved Hide resolved

zsusswein added 7 commits June 22, 2024 18:05

Grammar tweaks to dimensionality_heuristic()

59fac3f

Remove k documentation from RtGam()

feda446

And instead point to `dimensionality_heuristic()` as the sole source of guidance per @seabbs

Better explanation of why use piecewise for k

dafc329

Wording tweaks and line-length formatting

8957487

dimensionality_heuristic -> smooth_dim_heuristic

4a0e5db

To prepare also a user-specific penality basis dimension with associated heuristic default

Typo

c6b76cb

zsusswein mentioned this pull request Jun 24, 2024

Explore alternative penalty basis dimensionality defaults #19

Open

Drop language linking groups to geography

8783d78

zsusswein changed the base branch from 08-class-constructor to main June 24, 2024 13:54

pre-commit

d4010f6

seabbs reviewed Jul 2, 2024

View reviewed changes

R/formula.R Outdated Show resolved Hide resolved

seabbs approved these changes Jul 2, 2024

View reviewed changes

Drop obsolete comment

0e60e5e

zsusswein mentioned this pull request Jul 6, 2024

#8 [3/4]: Implement model fitting with mgcv #20

Merged

zsusswein and others added 24 commits August 29, 2024 10:24

Implement model fitting with {mgcv}

cf29e5c

Allow optional switching between gam() and bam(), both for functionality now and to illustrate how one might extend to different backends in the future.

Suppress public docs of internal function

d87f361

Add checks and warnings for unwise inputs

afa8102

Refactor to S3 methods for fitting backends

24d310c

Add doc for missing param

6d841d1

Explicitly namespace modifyList()

046d055

Clarify documentation

19ae8f0

Test warnings throw for suboptimal params

55ded03

Default args in S3 methods w/ user-supplied in ...

bb0c13d

It makes documentation slightly more straightforward

Move backend check from input val to S3 dispatch

318fa9a

Allowing future extensions.

Whitespace

5b1085d

Move do.call() outside of fit_model()

3962347

As suggested by @seabbs. This allows ... to be evaluated and handles dispatch.

Dynamically find methods for fit_model()

8591b8a

Minimal working print method + RtGam() return

93f4402

Has basic info but still needs model fit diagnostics

Add some basic diagnostic checks

e9f1911

Clean up existing tests

b19258f

Tests for print and diagnostics

6d2af44

Document check_diagnostics()

25d452e

Update R/RtGam.R

b965903

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>

pre-commit

84b8d92

Move {withr} to suggests

5a12ee8

DRY diagnostic functionality

0c849f6

By pointing the public function to the slot with the stored diagnostic list. Re-order `RtGam()` to use the public function for diagnostic reporting.

pkg::func() -> func() in @examples

6bfb09d

Rename format_for_return() -> new_RtGam()

eed7b48

zsusswein merged commit 2179858 into 08-class-constructor Aug 29, 2024
1 of 2 checks passed

zsusswein deleted the 08-model-fit-helpers branch August 29, 2024 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#8 [2/4]: Model fit helpers #18

#8 [2/4]: Model fit helpers #18

zsusswein commented Jun 12, 2024 •

edited

Loading

zsusswein commented Jun 12, 2024

codecov bot commented Jun 12, 2024 •

edited

Loading

seabbs left a comment

zsusswein commented Jun 12, 2024 •

edited

Loading

athowes left a comment •

edited

Loading

athowes commented Jun 12, 2024

zsusswein commented Jun 24, 2024 •

edited

Loading

seabbs left a comment

#8 [2/4]: Model fit helpers #18

#8 [2/4]: Model fit helpers #18

Conversation

zsusswein commented Jun 12, 2024 • edited Loading

zsusswein commented Jun 12, 2024

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

seabbs left a comment

Choose a reason for hiding this comment

zsusswein commented Jun 12, 2024 • edited Loading

athowes left a comment • edited Loading

Choose a reason for hiding this comment

athowes commented Jun 12, 2024

zsusswein commented Jun 24, 2024 • edited Loading

seabbs left a comment

Choose a reason for hiding this comment

zsusswein commented Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

zsusswein commented Jun 12, 2024 •

edited

Loading

athowes left a comment •

edited

Loading

zsusswein commented Jun 24, 2024 •

edited

Loading