Cleaner stack trace with visitors #982
Conversation
krlmlr
force-pushed
the
f-759-named-funs
branch
from
bc31d3a
to
00d46c2
Compare
Codecov Report
@@ Coverage Diff @@
## main #982 +/- ##
==========================================
- Coverage 91.07% 91.06% -0.02%
==========================================
Files 46 46
Lines 2713 2708 -5
==========================================
- Hits 2471 2466 -5
Misses 242 242
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if cb15e52 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
Do we need stack traces only for understanding where styler spends more time or do you want to merge that I to the default branch? Ie maybe it’s not necessary to go the slippery slope for the production code and this version of the code can reside in a side branch? |
|
We could go with two parallel files, and activate only one of them depending on some switch. Do you prefer that? Do I read the benchmark correctly: we're faster when applying the cache, but slower without cache? |
Well that means more code to maintain. What's exactly the need for clean(er) stack traces than what we have now? If the analysis of the traces is a more or less one-off thing, then I don't think we should introduce meta programming into the default branch. |
Yes, but you see the confidence interval contains zero, so it's not a significant change. Gray check boxes in {touchstone} just mean no significant deviation in either direction. |
|
The stack traces also affect error messages as in We're talking about the two functions |
|
"Not significant" might just mean "we need more data to be sure". I do believe that the meta code takes a tad longer overall. |
Our end users should not see these anyways. |
Other PRs are good yes, thanks again. |
|
Yes, you can run {touchstone} for more iterations, then you'll always get a statistically significant result. However, if the change is +0.1% with a confidence interval around it of < 0.001%, it's not really significant from a user experience point of view, although statistically different from 0. So we tried to run for as many iterations that the two significance dimension align (so if user experience is slowed down significantly, it also creates statistical significance). I believe from a speed perspective, the changes your PR introduces are not problematic. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2c2aaaf is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e222ace is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
krlmlr
force-pushed
the
f-759-named-funs
branch
from
d829284
to
08a8def
Compare
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if ec7ca0a is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if a98c069 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 940010d is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0592e8e is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
make_visit_one()We could extend this idea to implementing a
make_visit()function,pre_visit()would bevisitor <- make_visit(pre = TRUE)thenvisitor(pd_nested). This would save a few function calls, give more compact code, and also better stack traces forpre_visit_one()(which would vanish). The 80-20 rule forbids to do this right now, maybe later. Also, this turns the whole visiting logic into a metaprogram, this is a slippery slope.For #759.