conroylau / lpinfer

lpinfer: An R Package for Inference in Linear Programs
GNU General Public License v3.0
3 stars 5 forks source link

Be a bit less strict on data formats #28

Closed a-torgovitsky closed 4 years ago

a-torgovitsky commented 4 years ago

I tried to use a tibble from the widely used tidyverse and received an error. You should check what others do for this, but I suspect they just coerce whatever the user passes (data.table, matrix, tibble) into a data.frame and use that. But you should check!

Edit: Actually, I imagine coercing into a matrix is the right thing to do in this case, since we know all of the columns should be the same data type.

conroylau commented 4 years ago

Done! Now the code allows the data to be in the tibble format too.

a-torgovitsky commented 4 years ago

Did you just allow a new type? Or are you coercing the input into matrix form? I think the latter is probably better practice since then it also covers any other input type we haven't thought of, as long as that input type can also be coerced into a matrix. You should check to see what other R code does (e.g. lm).

conroylau commented 4 years ago

Ah, I see. What I did is that I am allowing a new type and forcing that into a matrix form. I agree coercing them in a data format is better than checking the class.

For lm, they require data to be a data.frame. It cannot be matrix or array. The same goes for rq and boot.

a-torgovitsky commented 4 years ago

Yes but lm does allow it to be a data.table or tibble as well. So what happens if you pass lm a tibble? Does it coerce to a data.frame?

conroylau commented 4 years ago

Yes if I pass a tibble or a data.table to lm, I think they coerce them to a data.frame via the code mf <- eval(mf, parent.frame()) where mf is the data passed to lm. (Here, mf is the object that can be obtained from the model component after calling lm).

The class of the mf object becomes data.frame no matter I pass a data.frame, data.table or tibble.

a-torgovitsky commented 4 years ago

I see, ok. Let's do something like that too, although here I guess we can go all the way to matrix since that's what Gurobi will take anyway.

conroylau commented 4 years ago

I see. Although they are taking data.frame, I am also thinking would coercing to a matrix be a better option for our case? Since we are mainly using data to construct the A.obs, A.tgt, A.shp etc. matrices if the user passes a function, and often we need to do some matrix multiplications in the code, I am wondering if it is better if we go all the way to matrix for the module? Thanks!

a-torgovitsky commented 4 years ago

Yes that's what I meant -- we should coerce to a matrix.

conroylau commented 4 years ago

Done! Just updated the code to coerce data to a matrix.

conroylau commented 4 years ago

As discussed in issue #30, now data is coerced to data.frame instead of matrix.