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

Don't extend base functions on base types #24

Merged
merged 2 commits into from
May 25, 2017
Merged

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented May 2, 2017

Please don't monkey-patch Base, that has global side effects

@tkelman
Copy link
Contributor Author

tkelman commented May 3, 2017

@MikeInnes hello?

@@ -13,5 +13,5 @@ end
_, ys = apply(unroll1(r).model, xs, (r.y.x,))
@test ys[1] == tanh(xs[1] * r.Wxy.x .+ r.y.x * r.Wyy.x .+ r.by.x)
ru = unroll(r, 3)
ru(batchone(Seq(squeeze.(xs))))[1] == squeeze.(ys)
ru(batchone(Seq((x->squeeze(x,1)).(xs))))[1] == (x->squeeze(x,1)).(ys)
Copy link
Member

Choose a reason for hiding this comment

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

This should be squeeze.(xs, 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course, not sure why I didn't use that first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MikeInnes
Copy link
Member

I think a nicer solution would be to add this method to Base, to be honest.

@tkelman
Copy link
Contributor Author

tkelman commented May 4, 2017

That should be done via a PR to base (then backported via Compat once merged), not potentially concealing bugs in unrelated code by doing it in some package. And I don't think it makes much sense given the docstring is

  squeeze(A, dims)

  Remove the dimensions specified by dims from array A. Elements of dims must be unique and within the range
  1:ndims(A). size(A,i) must equal 1 for all i in dims.

Unless there are many of these dimension arguments that default to 1 in other functions that I'm not thinking of.

@tkelman
Copy link
Contributor Author

tkelman commented May 4, 2017

Could give this a local squeeze1 name for the time being, or Flux.squeeze and not import it, to avoid side effects. Whatever you prefer that can avoid the type piracy.

@tkelman
Copy link
Contributor Author

tkelman commented May 11, 2017

Can we move on this? Changing the behavior of totally unrelated code depending on whether or not this package is imported is not okay.

@tkelman
Copy link
Contributor Author

tkelman commented May 21, 2017

Bump again, this would be incompatible with the proposed meaning in JuliaLang/julia#22000, all the more reason it shouldn't be extended here.

@MikeInnes
Copy link
Member

Perhaps it would be better for Flux to have its own, unexported squeeze with these semantics so it can be imported by users.

@tkelman
Copy link
Contributor Author

tkelman commented May 22, 2017

Sure, that works too, done.

@staticfloat
Copy link
Contributor

@MikeInnes fwiw, a bunch of the discussion in the above linked issue basically boiled down to me realizing that for my use cases, what I really wanted was to just use vec(), which takes in things like Nx1 matrices and spits out N length vectors. It's a little weird in that it can also take in NxN matrices and spit out (N^2) vectors, but in your use case that might not be a problem.

@MikeInnes
Copy link
Member

In this case it comes up because the first dim is a batch – but often you just want to work with one (N-d) sample, so it's convenient to wrap and unwrap that in a batch of size 1. So batch and unbatch do map to [un]squeeze(xs, 1) and there's no real way to get around that.

Of course, it'd be much nicer to have batching be first class in the type system rather than representing it implicitly like this, but that's another project.

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.

3 participants