Skip to content

Comments

Add support for sig_symbols to aov_car()#27

Merged
singmann merged 2 commits intosingmann:masterfrom
crsh:aov_sig_symbols
Apr 6, 2017
Merged

Add support for sig_symbols to aov_car()#27
singmann merged 2 commits intosingmann:masterfrom
crsh:aov_sig_symbols

Conversation

@crsh
Copy link
Contributor

@crsh crsh commented Apr 2, 2017

Hi Henrik,

better late than never (an unexpected personal event came up). This pull request adds the support for sig_symbols to aov_car() as discussed in #17.

Let me know what you think.

crsh added 2 commits April 2, 2017 23:50
- Adds new option afex.sig_symbols
- Adds sig_symbols attribute to anova_table
- Adds significance symbol legend printing to print.afex_aov()
@singmann
Copy link
Owner

singmann commented Apr 4, 2017

Thanks for that. Two thoughts though before I accept that:

  1. Why does sig_symbol not get an own argument in the anova method? That seems to be inconsistent with the other arguments that appear in the nice methods. Only sig_symbol now uses dots as far as I can see. (The exception seems to be intercept).

  2. Shouldn't sig_symbol also be passed through to the anova call in nice.afex_aov?

Sorry for the delay. I was working on the vignette, which is finally ready. So as soon as this thing is in it I will push to CRAN.

@crsh
Copy link
Contributor Author

crsh commented Apr 5, 2017

If I understand your questions correctly the following should answer both points.

Currently, anova.afex_aov() returns an anova object. Upon return that object is printed by stats:::print.anova(). That function, in turn, calls stats::printCoefmat() to generate the table, significance symbols and legend. However, stats::printCoefmat() does not accept a sig.sybmols argument. Hence, there is no sig_symbols argument because it cannot readily be used. After thinking some more about this, we could circumvent this problem. We could return the object invisibly and let, e.g., nice() do the printing. This approach would unify the appearance of the printed output of print.afex_aov() and anova.afex_aov() and is an easy (cosmetic) fix. Consequently, we still would have to pass sig_symbols to nice.anova() within the call to nice.afex_aov() (if that was your second question).

@singmann
Copy link
Owner

singmann commented Apr 6, 2017

The reason for the existence of the ANOVA method is twofold. One reason is that it allows to obtain the numerical values for $p$-values and stuff. The other reason is that there are some people who really like the standard anova output above the nice output. I am not one of them, but who am I to judge?
However, this also means that I think using nice for printing anova objects is not necessarily desired.

What I am gonna do is accept your changes and still add sig_symbol as argument to anova.afex_aov`. That seems to be most consistent with the way the function works (i.e., all things that can be specified are explicit arguments).

@singmann singmann merged commit ca3b8bc into singmann:master Apr 6, 2017
@singmann
Copy link
Owner

singmann commented Apr 6, 2017

Okay, I have now added the argument to anove.afex_aov and also use the value from afex_options in nice.mixed. I also needed to make a change to your sleg line which now uses stats::symnum. Otherwise R CMD check complained.

It would probably good to add the legends to the output of print.nice and print.mixed. Can you eventually give it a try?

@crsh
Copy link
Contributor Author

crsh commented Apr 7, 2017

That's the next thing, I'll be working on. I'll try to get to it before the end of the week.

@singmann
Copy link
Owner

singmann commented Apr 7, 2017

That sounds great. One important thing to note is that it is important to make sure that the new methods also work with objects created in the old version. For example, I needed to add a check for !is.null(attr(x$anova_table, "sig_symbols")) to print.afex_aov for it to not throw an error when printing old version objects (I added one such object to tests/testthat/afex_aov_16_1.rda).

In addition, I tried to upload the current version already to CRAN, but it creates a warning with a package downstream, so it will be somewhat more time until I can get the new version up...

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.

2 participants