Add aic dofv #731
Draft
Add aic dofv #731
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- These 3 columns can be turned off by passing `calc_aic_bic = FALSE` and `calc_dofv = FALSE` to the summary log implementation function (passed via `...`). This was done for bootstrap summarization, but may be necessary for other model types as well. - Note about dofv: adding support for calculating the difference between objective function values ended up being more complicated than originally thought. The reason being that we have to take nested directories into account. The prototype function simply used the run and based_on columns, but this doesnt work when searching recursively, because based_on specifiers would be displayed as '../2', which wouldnt show up in the run column. Fortunately we did already have a method for linking models, but _unfortunately_ it's tied up in the model_tree functions (specifically make_tree_data). This leads to some unnecessary computation. Refactoring the linking to be a separate helper function is out of scope for this PR, though that should probably be done at some point.
- subtracting `2` in the first case wasn't initially obvious. This would have to be adjusted if more columns overlap in the future. I considered adding `based_on` to the summary_log for clarity on `dofv`, but this presented additional issues that would have led to more invasive changes. - To the same effect, specify the column name in the second case. The index had to be changed when adding another overlapping column between the log types. To avoid uneccesssary fixes in the future, we just specify the intended column by name now.
|
Still need to write tests, but the prototype is complete. I think this is a good place to gather feedback before continuing. |
|
@callistosp can you take a look at how I calculated BIC when you get a chance? I think we'll want to have some more design discussions (from both a user/scientist and technical perspective) too, but wanted to confirm that part early on. |
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add AIC, BIC, and DOFV columns to the
summary_logmodel_treePR because certain fixes were needed in order to rely onmake_tree_data, which is used to link the models when calculatingdofv(see first commit for more details).closes #677