Prior transformation with jacobian#1495
Conversation
C++ changes
- create transformation enum struct and distribution enum struct in common/types.hpp
- create common/fims_transformations.hpp with transformation functions
- ApplyTransformation, ApplyBackTransformation, TransformPrior, AddLogJacobian
- restructure variable_map to hold transformation metadata for parameters
- add AddLogJacobian call to model.hpp and create do_mcmc flag
- add Prepare function to distributions_base to calculate prior transform for each iteration
Interface Changes
- add new functions: add_prior to ParameterVector and add_shared_prior to rcpp_interface.hpp
- add code to parse R formula and pass transformation information and prior informationinto the backend
- add set_variable_map function to pass values and transformation metadata from Rcpp to C++
- modify calls to variable_map and set input transformations if parameters are not logged
Tests
- add C++ tests for fims_transformation.hpp
- update R tests and vignettes
cleanup code
- remove min/max field and functions
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1495 +/- ##
==========================================
+ Coverage 82.97% 83.68% +0.71%
==========================================
Files 54 58 +4
Lines 2214 2464 +250
Branches 579 614 +35
==========================================
+ Hits 1837 2062 +225
- Misses 279 340 +61
+ Partials 98 62 -36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
See #431 for remaining tasks on draft PR |
| * - `exp`: Exponential transformation, parameter is on the log scale. | ||
| * - `log`: Log transformation, parameter is on the natural scale. | ||
| * - `logit`: Logit transformation with optional bounds. | ||
| * - `square`: Square transformation. |
There was a problem hiding this comment.
We could replace the square and sqrt transform options with a power option that takes an input value for the power square=2, sqrt=1/2 and this is then extendable to other powers.
| * | log | log(x) | | ||
| * | exp | exp(x) | | ||
| * | logit | logit(x, lo, hi) | | ||
| * | square | x^2 | |
There was a problem hiding this comment.
| power | power(x, power_value) |
| * specified by the transformation argument. The forward transformations | ||
| * and are: | ||
| * | ||
| * | Label | Forward | |
There was a problem hiding this comment.
I use x in the comment below for consistency with the other functions but we shouldn't use 'x' or any other similarly small variable names in the codebase as it makes interpretation and future refactors really difficult. Particularly with something like x being used to mean different things throughout the codebase.
| * @param prior The transformation applied to the parameter in prior space | ||
| * (e.g. square if the prior is on variance = sd^2). | ||
| * @return A vector of parameter values transformed to prior space. | ||
| */ |
There was a problem hiding this comment.
This could be extended to be more generalized and save compute time if it stores the input, natural, and prior transformed values somewhere for when they are needed throughout the code rather than calculating every time.
| * (e.g. square if the prior is on variance = sd^2). | ||
| * @return A vector of parameter values transformed to prior space. | ||
| */ | ||
| template <typename Type> |
There was a problem hiding this comment.
It would also make sense to extend this to simply a likelihood transformation rather than just limiting it to priors. For that we would need to tie the transformed values to the likelihood component of interest.
| @@ -139,10 +139,54 @@ class Information { | |||
| uint32_t, std::shared_ptr<fims_popdy::FisheryModelBase<Type>>>::iterator | |||
| model_map_iterator; /**< iterator for variable map>*/ | |||
|
|
|||
There was a problem hiding this comment.
To extend this to all likelihood components we would have to have one map linked to the input variables like this that holds the input transformation type and the input and natural values. It could also be helpful here to have a reporting transformation and value specified so that use gets things out in the desired way.
We would then need a second map linked to the likelihood components that specifies the transformation type and transformed values needed for each likelihood component. This allows parameters to have multiple transformations such as one for a prior and one for fit to observed data.
This approach would replace the manual log and natural parameters we currently specify manually in FIMS like log_F_mort and F_mort we would just specify which transformation is used in a specific internal calculation.
| return "unknown"; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This section on the distribution functions doesn't seem to fit with the rest of the refactor.
nathanvaughan-NOAA
left a comment
nathanvaughan-NOAA
left a comment
There was a problem hiding this comment.
Some initial comments. Don't need to address this stuff for this pull request but thoughts for how to improve on this in the future.
3 participants
What is the feature?
How have you implemented the solution?
Does the PR impact any other area of the project, maybe another repo?
Instructions for code reviewer
👋Hello reviewer👋, thank you for taking the time to review this PR!
nit:(for nitpicking) as the comment type. For example,nit:I prefer using adata.frame()instead of amatrixbecause ...This PR is now ready to be merged.Checklist