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

Fix transformation fit call with default variables #595

Merged
merged 2 commits into from
Aug 29, 2020

Conversation

wildart
Copy link
Contributor

@wildart wildart commented Aug 24, 2020

Fix #594

@wildart wildart changed the title fixed #594 Fix transofrmation fit call with default variables Aug 24, 2020
Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

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

Thanks, looks like we missed this one before. Did you double check that the other cases are handled in the same way?

@kleinschmidt
Copy link
Member

I wonder if we should back port this to the 0.32 series too (as with #565)

@wildart
Copy link
Contributor Author

wildart commented Aug 25, 2020

I made dims=1 to be the default value for vectors. Using nothing doesn't make sense in such case.

@kleinschmidt
Copy link
Member

Actually I think it's better to leave the default value as nothing, unless we want to make the dims= kwarg optional for vectors.

@kleinschmidt
Copy link
Member

The logic being that eventually (aka on the next breaking change release) there will be an error if dims is not specified.

@wildart
Copy link
Contributor Author

wildart commented Aug 26, 2020

there will be an error if dims is not specified.

After all, there is only one dimension in a vector. In future, this will be a default argument with value 1. It is not that, we cannot figure out the parameter type, so the dims is required. We still rely on multiple dispatch. I understand necessity of the dimension selection for a matrix argument, but not for a vector. mean doesn't require dims parameter for the vectors nor many functions. Why should any function operating on vectors require this parameter?

@nalimilan
Copy link
Member

nalimilan commented Aug 27, 2020

For consistency with Statistics, where cov and cor treat columns as variables, it would make sense to switch the default to dims=1 at some point after removing the deprecation. Then the default would also be correct for vectors. I'm hesitant about whether we should make this change for vectors now in anticipation of the general switch or not.

@wildart
Copy link
Contributor Author

wildart commented Aug 27, 2020

The functions with the matrix parameter have the warnings about dims parameter usage. But is this rely necessary to inform user use dims=1 parameter with the vector input? Will dims be a positional non-default parameter?

@nalimilan
Copy link
Member

No, dims will remain a keyword argument AFAICT, that's consistent with Base, Statistics and Distances. I don't have a strong opinion about vectors, so let's hear what others think.

@kleinschmidt
Copy link
Member

If dims=1 is going to be the default everywhere I have no problem making it the default for vectors as well; it's the only sensible choice for vectors so there's no sense in requiring users specify it. It's not even clear to me that having it as a kwarg is even necessary except for consistency with the matrix methods

@kleinschmidt
Copy link
Member

Also just so I'm clear on the overall plan here: for v0.34 we'll make dims=1 the default and remove the dims=nothing path, correct?

@nalimilan
Copy link
Member

Maybe first remove the deprecation, and wait for a release before switching the default?

@nalimilan nalimilan changed the title Fix transofrmation fit call with default variables Fix transformation fit call with default variables Aug 29, 2020
@nalimilan nalimilan merged commit ec33334 into JuliaStats:master Aug 29, 2020
@wildart wildart deleted the transform-fix branch September 1, 2020 10:49
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.

UndefVarError: l not defined when calling fit with dims = nothing using StatsBase v0.33.0
3 participants