Derivatives of transformations #341
Conversation
|
@mjskay would you update the PR to remove conflicts? |
|
I'd like to point out a small interaction with #360 here. |
|
if this goes in first, happy to update #360 to use this feature (and double check derivates etc) |
|
Conflicts resolved here. Thanks @teunbrand @pearsonca. Similarly, if #360 goes first I'd be happy to add those derivatives here; either way works for me. |
|
We'll do #360 first. Will let you know once merged |
oops, saw this after writing derivates into 360 - i can deal with rebasing that once this is merged? |
|
@pearsonca I think @thomasp85 was suggesting to merge #360 without the derivatives and then deal with derivatives here, which probably makes sense in case any feedback on this PR causes changes to the derivative stuff (e.g., name changes or what have you). |
|
#360 has been merged - can you update this PR with derivatives for that and then we can get this in |
|
Done! Also thanks for all the hard work shepherding through all these issues and PRs :). |
|
Thank you for all the work |
The latest release of the scales package has added a pair of optional arguments, `d_transform` and `d_inverse`, in the middle of the parameter list for `trans_new` (see r-lib/scales#341). As a consequence, this has shifted the position of the `breaks` parameter from 4th to 6th. Because the `bench_time_trans` and `bench_bytes_trans` functions from this package were passing in the breaks object positionally, this change in the scales package means that the breaks object is now matched with the new `d_transform` formal parameter and it ends up being ignored. This commit changes the arguments to `trans_new` from being positional to being named, which should be more robust to future changes to the function.
The latest release of the scales package has added a pair of optional arguments, `d_transform` and `d_inverse`, in the middle of the parameter list for `trans_new` (see r-lib/scales#341). As a consequence, this has shifted the position of the `breaks` parameter from 4th to 6th. Because the `bench_time_trans` and `bench_bytes_trans` functions from this package were passing in the breaks object positionally, this change in the scales package means that the breaks object is now matched with the new `d_transform` formal parameter and it ends up being ignored. This commit changes the arguments to `trans_new` from being positional to being named, which should be more robust to future changes to the function.
This PR closes #322 by supplying derivatives of transformation functions and inverses where possible.
Specifically, it:
NULLfields to transform objects:$d_transformand$d_inverse. These functions are the derivatives of$transformand$inverse.compose_trans()(unless any derivatives in the transformation are missing, in which case the corresponding derivative function isNULL).Some possible discussion points:
$transform_derivand$inverse_deriv, but found that existing tests used partial matching on$transand$inv, so these names caused failures. I wasn't sure if other legacy code relies on this partial matching, so I conservatively went with the names$d_transformand$d_inverse. I am not sure these are the best names and am happy to change them.identity_trans(), at least for applications like I am imagining these will be used for (plotting densities), but I went with the more conservative choice of not including it.