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

Update ggforest.R #388

Merged
merged 1 commit into from
May 20, 2019
Merged

Update ggforest.R #388

merged 1 commit into from
May 20, 2019

Conversation

DanChaltiel
Copy link
Contributor

This corrects the bug that made ggforest not displaying polynomial and spline terms, as seen in #306.
Of note, I'm confused about the lines 50-51, which are quite long to compute (because anova()) but do not seem to add much information (terms remained inchanged in all tests I could do). If they do serve some purpose, maybe it could be abstractized into some optional argument ?

This corrects the bug that made ggforest not displaying polynomial and spline terms, as seen in #306.
Of note, I'm confused about the lines 50-51, which are quite long to compute (because `anova()`) but do not seem to add much information (`terms` remained inchanged in all tests I could do). If they do serve some purpose, maybe it could be abstractized into some optional argument ?
@pbiecek
Copy link
Contributor

pbiecek commented May 20, 2019

Thanks! @kassambara would you merge this pull request?

@kassambara kassambara merged commit 38ab754 into kassambara:master May 20, 2019
kassambara added a commit that referenced this pull request May 20, 2019
@DanChaltiel
Copy link
Contributor Author

Cool, thanks ! @kassambara but what about lines 50-51? Could you tell me why you want to perform anova here?

@kassambara
Copy link
Owner

@pbiecek is the function anova() mandatory for extracting predictor variables in the cox model at line 50-51?

@pbiecek
Copy link
Contributor

pbiecek commented May 20, 2019

@kassambara @DanChaltiel thanks for spotting this
this line was added in pbiecek@1f9bc21#diff-97cd488e9c8b8ef57f4181c52a0dc79d

but now I do not see why it is needed.

I've removed this like and check() is passing, so I will create pull request that removes this line

pbiecek added a commit to pbiecek/survminer that referenced this pull request May 20, 2019
@kassambara
Copy link
Owner

ok, thanks!

kassambara added a commit that referenced this pull request May 20, 2019
removed unnecessary call to anova() as requested in #388
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.

3 participants