Conversation
bf09b58 to
d2c67ef
Compare
| /// - ``ones(_:type:stream:)`` | ||
| static public func zeros( | ||
| _ shape: [Int], dtype: DType = .float32, stream: StreamOrDevice = .default | ||
| _ shape: some Collection<Int>, dtype: DType, stream: StreamOrDevice = .default |
There was a problem hiding this comment.
I wonder how this wasn't ambiguous before? Maybe it was and it just picked one (since they have the same defaults)?
There was a problem hiding this comment.
I don't know either, I've come across with this issue a lot when I genericized my codebases. Maybe due to some type-checker quirks.
I think removing these default arguments won't break the call sites. We might, however, bikeshed on whether to remove these from type: or dtype:.
There was a problem hiding this comment.
I don't think it will break callers, but we can try it out with mlx-swift-examples and see if anything shows up.
There was a problem hiding this comment.
I just tested the current branch with mlx-swift-examples and everything built cleanly.
1ef6091 to
b03d202
Compare
|
One unintended consequence of this PR is it has broken a lot of docc function hashes... |
FWIW they seem very fragile -- I noticed quite a few warnings the last time I did a doc build locally. |
|
@AppAppWorks is this ready to go? It looks like it needs |
Functionality-wise this PR should be ready, however I am not sure if we should proceed without the documentation fixed. I wanted to fix the broken function hashes and was then carried away by schoolwork... |
bac63f0 to
1ff471c
Compare
|
After some thought, I suggest merging these changes in this PR while I'll open another PR to fix documentation and formatting glitches. |
554821b to
e2578dd
Compare
davidkoski
left a comment
There was a problem hiding this comment.
Thank you for this contribution!
|
Looks like it just need a run of the pre-commit (swift-format) |
- replaced concrete arrays with opaque Collection and Sequence to improve input genericity - no replacement when the generic parameter is an existential for the compiler cannot resolve for array literals - replaced generics with opaques for brevity - replaced existentials with opaques for more appropriate semantics - removed the default arguments for `dtype:` to resolve the ambiguities arisen after the changes
e2578dd to
128aa02
Compare
fixes #286
The blast radius was larger than I expected, I ended up implementing the following changes:
some Collectionandsome Sequence, whenever applicableSome type checking ambiguities arose after the changes, I have to do the following to break ties:
dtype:from functions which have atype: T.Type = <default>overloadI've avoided overridable methods to maintain source compatibility