search

madeleineudell / LowRankModels.jl

LowRankModels.jl is a julia package for modeling and fitting generalized low rank models.
Other
190 stars 65 forks source link

Logistic() overwritten by using DataFrames #41

Closed ahwillia closed 9 years ago

ahwillia commented 9 years ago

Just wanted to highlight this annoyance:

using LowRankModels
using DataFrames
logistic(1.0) # returns 0.731 instead of logistic(1.0,BoolDomain())
logistic() # ERROR: `logistic` has no method matching logistic()

This is the problematic function: https://github.com/JuliaStats/StatsFuns.jl/blob/9991e25012e29cf4bc1f081eec4ed9a1a6874291/src/basicfuns.jl#L16

I'm not sure why DataFrames needs to import all of StatsBase... but seeing as many users will likely be using one or both of these packages in conjunction with LowRankModels this could be a potential problem.

Should we consider changing names: logistic() ==> logloss(), quadratic() ==> quadloss(), etc...

madeleineudell commented 9 years ago

That's bad news. The idea is that StatsBase forms a common base for all packages using statistics, so no one overwrites any function in there. We can get around it by importing all of StatsBase ourselves (which might be advisable to prevent this kind of issue in the future), and/or by changing the names of our functions.

Since we want to be able to call logistic(1.0) and have it return a logistic type, I guess we should change the names --- multiple dispatch can't save us here. And as long as we're changing names, how about going to CamelCase names (LogisticLoss) to be in keeping with conventions for other types?

Madeleine Udell Postdoctoral Fellow at the Center for the Mathematics of Information California Institute of Technology www.stanford.edu/~udell (415) 729-4115

On Mon, Sep 14, 2015 at 5:05 PM, Alex Williams notifications@github.com wrote:

Just wanted to highlight this annoyance:

using LowRankModelsusing DataFrameslogistic(1.0) # returns 0.731 instead of logistic(1.0,BoolDomain())logistic() # ERROR: logistic has no method matching logistic()

This is the problematic function: https://github.com/JuliaStats/StatsFuns.jl/blob/9991e25012e29cf4bc1f081eec4ed9a1a6874291/src/basicfuns.jl#L16

I'm not sure why DataFrames needs to import all of StatsBase... but seeing as many users will likely be using one or both of these packages in conjunction with LowRankModels this could be a potential problem.

Should we consider changing names: logistic() ==> logloss(), quadratic() ==> quadloss(), etc...

— Reply to this email directly or view it on GitHub https://github.com/madeleineudell/LowRankModels.jl/issues/41.

ahwillia commented 9 years ago

Closed by https://github.com/madeleineudell/LowRankModels.jl/pull/42