-
Notifications
You must be signed in to change notification settings - Fork 23
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
Cannot coerce class ‘"function"’ to a data.frame #158
Comments
Hi @Fernal73 correct me if I am wrong but looks like this error is generated only when executed from the command line. I will look into this but might need your help as my command line knowledge is limited. |
I'm running my scripts on the command line on ArchLinux on Termux. I see no reason why they wouldn't run otherwise. I have not encountered any problems running my scripts specifically from the command line. None of them employ shiny. I'd appreciate it if you could assist since your package is quite comprehensive in covering step wise regression using different selection criteria. R version 3.6.2 (2019-12-12) -- "Dark and Stormy Night" olsrr version: 0.5.3 |
Sure.. I will get back to you at the earliest |
That's fine, Aravind. Else, I simply will have to write my own if I can't find any other package or script that does the same. Why reinvent the wheel? Am I missing something obvious? I find it hard to believe that a package like yours would have an obvious showstopper after being used extensively by your users. That's why I hesitated to write up this issue report. I've used leaps, MASS and step using AIC and written my own function for p_value as criteria. |
I think it should work if you can read the data as anything other than
|
I tried it with table as the variable name but encountered the same error. The variable name 'cement' worked as you pointed out. plot(k) provides no graphical output, however. The script can be found at: https://github.com/Fernal73/LearnR/blob/development/Stats462/Lesson11/cementolsrr.R |
Yes.. looks like that. Let me debug this and get back to you. |
Hi @Fernal73 The error was due to the way we were extracting data from the model:
and let me know if the below code works (the plots were printed in RStudio after I removed
|
You're returning a list of ggplot objects in k. To output that in a non-interactive environment, you would have to print the plots in a loop using print() or You might wish to check out the method print_chart in the following script if you need assistance: https://github.com/Fernal73/LearnR/blob/development/CoronaVirus/worldanalysis.R |
The plots are printed by default i.e. the https://olsrr.rsquaredacademy.com/reference/ols_step_all_possible.html |
print_plot does not work in a non-interactive environment when writing to a pdf document. I can work with the list of plots you return and output them sequentially. That works for me. However, your code must work in both modes. The graphical output especially when print_plot is TRUE must happen irrespective. You are using print() elsewhere for ggplot objects. Thanks for your assistance. As for installing from Github, I'd rather wait till you release your next version since it impacts my installer.R and I'd rather not have a longer install time than necessary. |
Thanks for bringing this to my notice @Fernal73.. it will help other end users very much. Will keep you posted about the next release. |
https://online.stat.psu.edu/stat462/node/196/ I've been referring to the above for step wise regression using p-values, Alpha-To-Remove and Alpha-To-Enter. The ols_step_both_p function does not use the above methodology. Is there any intention to provide the same? |
It does follow the methodology mentioned in the link. I have reproduced the example mentioned below:
|
Ok. I seem to have overlooked that.
The default prem = 0.3.
So that changes the result.
https://www.rdocumentation.org/packages/olsrr/versions/0.5.3/topics/ols_step_both_p
|
Yes. |
Another question wrt to the data.frame returned by ols_step_all_possible: |
Yeah.. we can replace the predictors column with model formula. How about returning https://stackoverflow.com/questions/33627179/how-do-i-store-lm-object-in-a-data-frame-in-r You know that way, users can directly use the |
The simplest thing to do is add an extra column for the response variable.
(Yes, it's redundant over multiple rows but sometimes adding redundancy is
the best solution).
Changing the predictors to formula will upset your existing users if they
depend on the existing structure of the field.
I did not consider returning lm objects.
I'd like my return value to be as lightweight as possible.
If you still want to do that, you can add an option to the method call such
as fill.models = TRUE.
Returning a list (including the data frame and models) or your original
data frame based on a parameter such as above makes it more flexible.
|
Thanks for your thoughtful inputs @Fernal73. I will think it over and implement the changes. |
I'd be chary of returning models over all possible combinations of variables in the model. It would be fine in the case of best_subsets which is a much smaller number. Either way, providing a choice such as fill.models is wise. |
I finally managed to get around to installing your package from Github. However, I'm still receiving the same error. Additionally, if I use a different name say datum or iqsize, I receive the following error message: "Error in eval(model$call$data) : object 'datum' not found The script is: https://github.com/Fernal73/LearnR/blob/development/Stats462/Lesson11/iqsizesubsets.R The exact same code in the following script works as expected: The only change is the variable name. So is the eval finding your data set 'cement' instead? You need to pass in the parent environment to eval to have it find the right dataset. |
Hi @Fernal73.. I am unable to reproduce the error. I tried importing the data using different names and it did not throw the error. Not sure why I used I used Again, thanks a ton for your detailed feedback :) |
cdedb9a#commitcomment-38881126 Are you expecting your users to run your code only in the RStudio environment? I've run your code with ols_step_all_possible. It works. Those changes have to be in ols_step_best_subset as well. That is the best way to subset the models and then refine the selection process further. Another point: You are setting the class for the returned data.frame object as per each function name. A better , more sensible convention would be to set each differently based on the structure of the data.frrame or object returned. That's more important to the user of your API. https://github.com/Fernal73/LearnR/blob/development/Stats462/Lib/libstep.R |
A user might wish to specify a starting model, other than all the fields in a dataset. |
We do expect the code to run in environments other than RStudio but can't assure the same. Regarding setting of class, we do that whenever a print method is defined. Not all the variable selection methods return just a We will replace |
Yup.. we hope to implement it in the next release. Will look into the API of lmSubsets. |
The data.frame structure returned in the above two method calls is identical. I'd expect the class for the data.frame to be the same. I don't want to accept a generic data.frame as the parameter to my functions since the implementation depends on the column names in your data.frame object. Rather than have my method crash, I'd rather stop processing if it isn't the class type expected. Fair.. we do need to review the output from different functions in olsrr so that the end user can use it for further analysis in a seamless manner. We will do this on a priority basis in the next release. Thanks for your input again :) |
I expect my code to work from the command-line on all R environments. |
This appears as though I've made this comments, not you. Not the right way to edit comments. It can be misleading. Yes, I agree. We don't have access to all environments. We do our best. |
Sorry @Fernal73 .. that was not intentional. I have removed that comment now but not sure how to add it back in the right place. |
Ditto about the comment again. I'd rather have a 'base class' added for the data.frame structure that returns all the criteria values such as Rsquare, adjr etc.... |
But will that still work if the end user would want to return the model objects for further analysis? My initial thought was to return a list which would include the |
@aravindhebbali R is not a strongly typed language. You can return two different class objects if they have different structures. Is there anything in R best practices that prevent you doing that as long as it's well documented? At some point of time, breaking backwards compatibility is the only way to go forward. As I said earlier, it can be staggered out over a couple of releases with adequate warning to users. Do you want to support multiple class objects in perpetuity? |
Not as far as I know. |
All said and done, when do you think will the changes about lm$model be propagated to all the methods using model_data? |
I have pushed the changes to the develop branch. |
https://github.com/rsquaredacademy/olsrr/blob/master/R/ols-best-subsets-regression.R But not to the above... |
I have a few more changes to make.. will merge them into the master branch later. |
Does this work with any other dataset other than the 'cement' one provided by olsrr? It does not. |
Please file a separate issue with a reproducible example. |
Aravind, all these errrors stem from the use of eval in model_data.
Replace that with model$model or use the parent.frame environment as a
parameter to eval to avoid the problem.
I wouldn't recommend the use of eval unless it's absolutely necessary.
If you can control the call to lm which fills the model$model with the
source data.frame, that's the way to go.
I am unaware of why you're specifically using eval. I'm not an R expert,
not yet.
The advice about eval holds.
You don't need a new issue for this. The original issue originates from the same problem.
|
Hi @Fernal73 We will not be using |
I can raise an issue for this. |
And this... |
Correct me if I an wrong but the below modification will return ggplot objects irrespective of the value assigned to print_plot and that is what you are suggesting Change from
to the below:
|
Yes, it can be returned invisibly. |
The default is for lm to return the dataframe in lm$model as the parameter model is set to TRUE by default. As I said, if you can control the call to lm, this behaviour is fine. If on the other hand, lm is invoked with model = FALSE, the data.frame will not be returned. |
The way I see it, these are the options.
Is there a fourth option? Should the call object be investigated further? I'm not familiar with its dynamics. |
As far as I can see, there isn't one. |
Brief description of the problem:
I try to run the following code and receive the following error message:
cement.txt
Assistance will be appreciated.
The text was updated successfully, but these errors were encountered: