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

Change identifer of stderr to se #368

Merged
merged 3 commits into from May 3, 2018
Merged

Change identifer of stderr to se #368

merged 3 commits into from May 3, 2018

Conversation

yukota
Copy link
Contributor

@yukota yukota commented Apr 8, 2018

Identifer confilict between meaning of IO and statical.
JuliaLang/julia#26636

@codecov
Copy link

codecov bot commented Apr 8, 2018

Codecov Report

Merging #368 into master will increase coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
+ Coverage   90.39%   90.51%   +0.11%     
==========================================
  Files          19       19              
  Lines        2020     2024       +4     
==========================================
+ Hits         1826     1832       +6     
+ Misses        194      192       -2
Impacted Files Coverage Δ
src/StatsBase.jl 100% <ø> (+50%) ⬆️
src/deprecates.jl 0% <0%> (ø) ⬆️
src/statmodels.jl 46.53% <0%> (ø) ⬆️
src/scalarstats.jl 95.83% <0%> (+0.01%) ⬆️
src/weights.jl 89.95% <0%> (+0.04%) ⬆️
src/hist.jl 95.45% <0%> (+0.5%) ⬆️
src/cov.jl 100% <0%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cf13cd...cf91012. Read the comment docs.

@Nosferican
Copy link
Contributor

Seems good.

@kmsquire
Copy link
Contributor

kmsquire commented Apr 8, 2018

As suggested here, how about stderror instead, to make it easier for new users (and old) to understand the meaning at a glance?

@Nosferican
Copy link
Contributor

The Slack channel favored se over stderror as it was deemed weird to abbreviate the first part and not the second one. se was also believed to be quite common and understood in the Statistical Model domain. The reason for not going too short (e.g., two letters symbol) was mostly to avoid another df for dof which conflicted with a common variable name for DataFrame structs. However, that does not seem to be the case with se.

@kmsquire
Copy link
Contributor

kmsquire commented Apr 8, 2018

I guess that’s a downside of trying to make decisions like this in multiple places.

Slack is great for help and discussions, but the fact that the history disappears after a short while means that there’s no permanent record of how decisions like this came to be.

Out of curiosity, did any of the (8) individuals who gave a 👍 on @stevengj’s comment (linked above) participate in the discussion?

I still think that stderror is more easily understood than se (which is important), and that in general, two letter function names are just a bad idea.

@ararslan
Copy link
Member

ararslan commented Apr 8, 2018

FWIW I don't think we should use se here, especially since our standard deviation function isn't sd.

@Nosferican
Copy link
Contributor

Nosferican commented Apr 8, 2018

I totally agree about Slack not being the best for these kinds of decisions. The best place probably (would have/is) an issue on StatsBase.jl This is an excerpt form the Slack discussion on the sd argument.

Andreas Noack [4:14 PM]
Actually I think that `se` would be fine if it wasn’t for `std` and `ste` looks really
weird to me. We have time for a renaming of `std` to `sd`, though, and…
Milan Bouchet-Valat [4:15 PM]
I've never liked `std`. It's an inconsistent abbreviation (STandard Deviation?),
and AFAIK the standard one is SD.

which led to the conversation about whether to lift the ban on using two-letter functions.
Given the above, I would favor having the PR modified to depreciate std in favor of sd as well as stderr for se.

@ararslan
Copy link
Member

ararslan commented Apr 8, 2018

I'd actually prefer we go in the opposite direction and use a longer name for standard deviation, e.g. stddev.

@Nosferican
Copy link
Contributor

stddev would then be inconsistent with stderror as error would not be abbreviated as deviation. It doesn't seem there is a consensus on how to proceed. Gonna open an issue about std and stderr and we revisit the PR with the decision there.

@Nosferican
Copy link
Contributor

After a couple days of discussion in the repo, I think the consensus it to change stderr to stderror. Since stderr is taken in Base, I don't know if the depreciation is worthwhile (overloading the value). Either way, the change will be reflected in the documentation.

@@ -20,7 +20,7 @@ import Base.varm, Base.stdm
@deprecate AICc(obj::StatisticalModel) aicc(obj)
@deprecate BIC(obj::StatisticalModel) bic(obj)

@deprecate stderr(obj::StatisticalModel) se(obj)
@deprecate stderr(obj::StatisticalModel) stderror(obj)
Copy link
Contributor

@stevengj stevengj Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like

if !isdefined(Base, :stderr)
    @deprecate stderr(obj::StatisticalModel) stderror(obj)
else
    @deprecate (io::IO)(obj::StatisticalModel) io === stderr ? stderror(obj) : throw(MethodErrror(io, (obj,)))
end

so that it still mostly works in 0.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing.
The second deprecate message cause syntax error.

ERROR: LoadError: LoadError: invalid usage of @deprecate

I'm tring to fix it, but it is difficult for me.
Could you tell me how to write?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to work around this limitation by doing something like:

function (io::IO)(obj::StatisticalModel)
    Base.depwarn("stderr(obj::StatisticalModel) is deprecated, use stderror(obj) instead", :stderr)
    io === stderr ? stderror(obj) : throw(MethodErrror(io, (obj,)))
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

yukota and others added 3 commits April 12, 2018 22:42
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

if !isdefined(Base, :stderr)
@deprecate stderr(obj::StatisticalModel) stderror(obj)
else
function (io::typeof(stderr))(obj::StatisticalModel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that typeof(stderr) can change at runtime if redirect_stderr is called.

So this should just be ::IO to catch all usages. I don't think it is a problem to have a fairly general method here as a temporary deprecation since it continues to throw a MethodError for non-stderr objects. (People are unlikely to pass StatisticalModel objects to random IO objects by accident anyway.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed ::IO because it doesn't work on 0.7. I'm not sure we should care about supporting that deprecation after calling redirect_stderr... Anyway I don't have any solution to suggest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't IO work? On 0.7 (unless something has changed very recently), typeof(stderr) is TTY, which is a subtype of IO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should care about supporting that deprecation after calling redirect_stderr

This would mean that the deprecation wouldn't work in IJulia, for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of JuliaLang/julia#14919:

julia> (io::IO)() = 1
ERROR: cannot add methods to an abstract type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see… yes, I can't think of a way to make it work for IJulia (without adding an IJulia dependency), since IJulia redirects to its own IO type.

@nalimilan nalimilan merged commit b01dcdf into JuliaStats:master May 3, 2018
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.

None yet

6 participants