Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev #50

Merged
merged 55 commits into from
Feb 14, 2020
Merged

dev #50

merged 55 commits into from
Feb 14, 2020

Conversation

DominiqueMakowski
Copy link
Member

@DominiqueMakowski DominiqueMakowski commented Sep 20, 2019

DO NOT MERGE THIS BRANCH cause it's like super broken, related to the reorganization with effectsize

Fix progress

@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5a5f9a9). Click here to learn what that means.
The diff coverage is 58.88%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #50   +/-   ##
=======================================
  Coverage          ?    45%           
=======================================
  Files             ?     29           
  Lines             ?   1382           
  Branches          ?      0           
=======================================
  Hits              ?    622           
  Misses            ?    760           
  Partials          ?      0
Impacted Files Coverage Δ
R/format_algorithm.R 0% <ø> (ø)
R/format_model.R 92.3% <ø> (ø)
R/report_model.R 0% <0%> (ø)
R/report_model_regression.R 0% <0%> (ø)
R/report_regression.R 0% <0%> (ø)
R/report_performance.R 0% <0%> (ø)
R/report_sample.R 0% <0%> (ø)
R/model_table_regression.R 0% <0%> (ø)
R/report_parameters_regression.R 0% <0%> (ø)
R/report_intercept.R 0% <0%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5f9a9...47953f8. Read the comment docs.

R/model_table.R Outdated
# Parameters -----------------------------------------------------------------
if(bootstrap & !info$is_bayesian){
if(is.null(ci_method) || ci_method %in% c("wald", "boot")) ci_method <- "quantile" # Avoid issues in parameters_bootstrap for mixed models
parameters <- parameters::model_parameters(model, ci = ci, bootstrap = bootstrap, iterations = iterations, p_method = p_method, ci_method = ci_method, ...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p_method and ci_method are now integrated in df_method.

@DominiqueMakowski
Copy link
Member Author

Let's get this big overhaul started 💪

So, as we discussed somewhere, the first step is to get that function that would create the big table (that basically merges the parameters, effectsize and performance tables).

I started the model_table() function, but it'd probably need an improved print method (knowing that we'd likely need to keep in mind that people will want to easily convert this table output to markdown/latex/knitr::kable). So we have to make it quite flexible.

Once we have that model_table() function working for our models (which should be straightforward, as really all the work is done in parameters and performance), we can start thinking about how best convert this table to a textual output.

@strengejacke @mattansb what do you think?

@strengejacke
Copy link
Member

Sounds good! Basically, I imagine something like these tables, but as textual output which can be easily integrated in non-HTML documents.

@DominiqueMakowski
Copy link
Member Author

DominiqueMakowski commented Feb 3, 2020

Yup, would it work to have different .print_report_table_* internals, such as print_report_table_console (default to console with coloured output), print_report_table_html(), print_report_table_latex() etc.?

@humanfactors

@strengejacke
Copy link
Member

I have no idea about latex... I think we should have an idea in mind how our table looks like. Then we have a data frame that "mimics" this table (with empty "cells" where the table cells should be empty as well), and then we have functions like add_first_column() or add_header_row() or whatever, and then, it either inserts HTML, or text/markdown. But this is just a quick thought, haven't thought about it in detail yet.

@strengejacke
Copy link
Member

We may also think of adding "layer" (but with +, not %>%, as the latter makes no sense when we actually add something).

@mattansb
Copy link
Member

mattansb commented Feb 3, 2020

The idea of focusing on tables first sounds good, as most of our outputs are data.frames anyway.
Might be interested to have a look at https://github.com/rstudio/gt (which I haven't used yet).

@DominiqueMakowski
Copy link
Member Author

python style > R style tho 🙊

The last big step, before merging to master, is to get that flexible yet powerful report() function for regression models. The tricky part is to find the sweet spot between flexibility (having methods that can adapt based on the input) and complexity (avoid having tons of ifelses for all the possible cases). And that's quite a challenge

@strengejacke
Copy link
Member

before merging to master

Why? Master is currently broken, while dev should be working better. Wouldn't it make sense to have the more stable branch as master?

@DominiqueMakowski
Copy link
Member Author

Alright so for regressions and complex models, model_text() (which creates the textual output), will be made from the combination of subparts, such as report_model(), report_intercept() and report_parameters() etc.

It'll be simpler to maintain these smaller chunks

@strengejacke
Copy link
Member

Sounds good. Should we merge this branch to have something working at master? We could encourage users to test this new version.

@DominiqueMakowski
Copy link
Member Author

Soon soon, hopefully by tomorrow, once a basic version of report for regressions works

@strengejacke with the new design I am not sure how to pass down arguments such as `width` (so that the text has a given max width), because in `text_short()` (in report.R) for example, the function just returns the `model_text` object which itself takes charge of printing 😕
my bad, `width` works but must be put it in the print function
@DominiqueMakowski
Copy link
Member Author

Not sure if I'm satisfied by the names of table_long, table_short, text_long, text_short... Opinions?

Maybe something like text_summary / text_details?

@DominiqueMakowski
Copy link
Member Author

Let's merge this, cannot be more broken than it is anyway so...... 😅

@strengejacke
Copy link
Member

merge_python

@strengejacke strengejacke merged commit 078cc17 into master Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants