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

consistently accept input shape as a static array only #337

Closed
timotheecour opened this issue Sep 13, 2016 · 9 comments
Closed

consistently accept input shape as a static array only #337

timotheecour opened this issue Sep 13, 2016 · 9 comments

Comments

@timotheecour
Copy link

timotheecour commented Sep 13, 2016

I'd like mir to consistently accept input shape argument as a static array, never as separate indexes, kind of like numpy:

a=numpy.zeros((2,3))//ok
a=numpy.zeros(2,3)//bad

Currently in mir this all works:

  auto a0=iota(3*4).sliced(3,4); // proposal:deprecate
  auto a1=iota(3*4).sliced([3,4]);
  auto a2=iotaSlice(3,4); // proposal:deprecate
  auto a3=iotaSlice([3,4]);
  auto a4=slice!int([3,4]);
  auto a6=slice!int(3,4);// proposal:deprecate
  // a bit inconsistent: only static array form is allowed when passing an extra argument
  auto a7=slice([3,4],5); 

Several reasons for changing this:

  • simpler interfaces when writing code
  • less template bloat
  • one obvious way is better than multiple ways (pythonic principle)
  • makes it possible to add more arguments after shape parameter, see example auto a7 above
  • easier to pass shape as an argument when it's a static array (doesn't require tuple unpacking)
@9il
Copy link
Member

9il commented Sep 14, 2016

I am not against it, but need more opinions about this.

@9il 9il added the API label Sep 14, 2016
@John-Colvin
Copy link
Contributor

I'm in favour of this, because users can always use [myDimsAliasSeq] to get the array (which won't allocate if it's bound to a static array symbol i.e. in the call to slice). I'm a bit worried about people creating a lot of garbage accidentally when manipulating dimensions though.

I think the 1D case should perhaps stay working without being an array, would seem like unnecessary noise to add [] there.

@9il
Copy link
Member

9il commented Sep 17, 2016

But all other functions ? Like transposed and iotaSlice

@John-Colvin
Copy link
Contributor

John-Colvin commented Sep 17, 2016

i think yes. Consistency is good here. Also, being able to add extra arguments in the future will help maintain flexibility in the future (very important, particularly once ndslice is out of std.experimental)

@9il
Copy link
Member

9il commented Sep 17, 2016

Ping DCV contributors @ljubobratovicrelja @henrygouk @DmitryOlshansky. What is your opinion about this breaking change?

@ljubobratovicrelja
Copy link
Member

ljubobratovicrelja commented Sep 17, 2016

I personally like the variadic approach, and would not forcibly replace it with static array. Say for the slice!T, as tensor allocation - most of the time I'd just allocate a tensor, without explicit value initialization - so [] would bother me in syntax mostly. Also, for e.g. I feel having [] in transposed would be a bit odd, in contrast to the present API.

I think I'm already used to the present API, which mostly works with variadics, and I haven't had any trouble with it - I really cannot tell how I would feel with newly proposed syntax. That's just my subjective view on it.

But, obviously since python (numpy) users feel the inconsistency, I suppose there's a real trouble here. Might be good idea to ping some more people, or even have the forum thread about it? (or maybe not the forum thread since we've seen how the simplest debate can burst into flames there...)

@John-Colvin
Copy link
Contributor

Hmm I think actually there's a distinction between arguments that are shapes (e.g. argument to sliced) and arguments that are dimension indices (e.g. arguments to transposed). I think static arrays are better for shapes. I'm undecided on dimension indices but would err on the side of using veridical, if only to make the distinction clear.

@9il
Copy link
Member

9il commented Oct 2, 2016

@9il 9il added the wontfix label Oct 7, 2016
@9il
Copy link
Member

9il commented Oct 7, 2016

According to the forum discussion (appr. equal Yes/No and small activity) I close this issue with wontfix.

@9il 9il closed this as completed Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants