[CoreML] Add Xcode 9 Beta 1 bindings #2275

dalexsoto · 2017-06-29T22:49:14Z

No description provided.

monojenkins · 2017-06-29T23:35:00Z

Build failure

monojenkins · 2017-06-29T23:45:14Z

Build failure

timrisi · 2017-06-29T23:59:07Z

src/coreml.cs

+	[Watch (4,0), TV (11,0), Mac (10,13, onlyOn64: true), iOS (11,0)]
+	[BaseType (typeof (NSObject))]
+	interface MLDictionaryFeatureProvider : MLFeatureProvider {
+


Whitespace. Same applies to the rest of the new interfaces

I always leave a Whitespace between the interface line definition and the first member definition and this is the first time I've been called out :)

a single empty line is fine to delimit code and help readability
whitespace changes are specially bad when updating files since they make larger diffs and contaminate git history/blame

timrisi · 2017-06-30T00:02:00Z

src/coreml.cs

+		bool Optional { [Bind ("isOptional")] get; }
+
+		[Export ("isAllowedValue:")]
+		bool IsAllowedValue (MLFeatureValue value);


Should this just be IsAllowed? Not as clearcut as some other cases.

timrisi · 2017-06-30T00:04:43Z

src/coreml.cs

+
+		[Static]
+		[Export ("undefinedFeatureValueWithType:")]
+		MLFeatureValue GetUndefinedFeatureValue (MLFeatureType type);


'Get' sounds wrong since this is still creating and returning an MLFeatureValue same as the other static methods. FromType doesn't work since it doesn't tell you that it's an UndefinedFeatureValue, but maybe just UndefinedFeatureValueFromType? or even CreateUndefinedFeatureValue

mmm I can't just use UndefinedFeatureValueFromType since it needs to start with a verb, but yeah CreateUndefinedFeatureValue sounds about right

I think I'll just go CreateUndefined since FeatureValue is already on the type name you are creating

timrisi · 2017-06-30T00:07:34Z

src/coreml.cs

+		[Export ("dataType")]
+		MLMultiArrayDataType DataType { get; }
+
+		[Export ("shape")]


Could/should this use BindAs (does BindAs support NSNumber arrays)?

Unfortunately, docs are not clear on the type we should use here nor is swift
var shape: [NSNumber]

timrisi · 2017-06-30T00:07:40Z

src/coreml.cs

+		[Export ("shape")]
+		NSNumber [] Shape { get; }
+
+		[Export ("strides")]


Unfortunately, docs are not clear on the type we should use here nor is swift
var strides: [NSNumber]

monojenkins · 2017-06-30T00:55:24Z

Build failure

dalexsoto · 2017-06-30T00:55:29Z

@timrisi Done :)

spouliot · 2017-06-30T01:40:48Z

src/coreml.cs

+	[Watch (4,0), TV (11,0), Mac (10,13, onlyOn64: true), iOS (11,0)]
+	[BaseType (typeof (NSObject))]
+	interface MLDictionaryFeatureProvider : MLFeatureProvider {
+


a single empty line is fine to delimit code and help readability
whitespace changes are specially bad when updating files since they make larger diffs and contaminate git history/blame

spouliot · 2017-06-30T01:43:25Z

src/coreml.cs

+
+		[Export ("objectForKeyedSubscript:")]
+		[return: NullAllowed]
+		MLFeatureValue ObjectForKeyedSubscript (string featureName);


that's already exposed by this [string] right ?
I don't mind having a method (easier to discover) but it should be named like one ;-) as this one lack a verb/action
e.g. GetValue (string)
The alternative is to mark it as [Internal]

That might be identical to GetFeatureValue from the protocol.
If that's the case (test needed) then it should be named identically (in fact it should not be bound as the generator will include it).

spouliot · 2017-06-30T01:46:30Z

src/coreml.cs

+
+		[Static]
+		[Export ("undefinedFeatureValueWithType:")]
+		MLFeatureValue CreateUndefined (MLFeatureType type);


Any reason not the name it FromUndefined to match other static methods ?

Br my reading, It's creating an undefined feature value from a feature type. Normally it'd just be "fromThpe", but needs something to indicate the "undefined" part

@spouliot What @timrisi said, just extending a little

The selector name is undefinedFeatureValueWithType: which from my reading states that you will get an undefined feature from the given type so FromUndefined from my reading says that what is undefined is the type parameter, not the object I am getting back that is why I think CreateUndefined matches a little better the intent of the above selector (give me an undefined object with the following type).

Make sense but let's double check if Apple documentation is available (and match).

From docs:

Declares a value as undefined.

From Headers:

Represent an undefined value of a specified type

@spouliot should I change it? I think the later makes sense with CreateUndefined. Naming is hard heh.

You are right :) Create makes more sense, it's the output (not in input) that's undefined.

spouliot · 2017-06-30T01:47:16Z

src/coreml.cs

+		MLFeatureValue FromDictionary (NSDictionary<NSObject, NSNumber> value, out NSError error);
+
+		[Export ("isEqualToFeatureValue:")]
+		bool IsEqualTo (MLFeatureValue value);


double check but IIRC we tend to use IsEqual in recent/newer bindings

monojenkins · 2017-06-30T02:46:55Z

Build failure

dalexsoto · 2017-06-30T18:37:31Z

@spouliot fixed! thanks!

spouliot

few more things to check ::)

spouliot · 2017-06-30T18:45:12Z

src/coreml.cs

+	}
+
+	[Watch (4,0), TV (11,0), Mac (10,13, onlyOn64: true), iOS (11,0)]
+	[Static]


Any reason for not adding [Internal] on them ?

I think they are more discoverable than the indexers but if you want them internal I'll follow suite :)

are they used somewhere else than the strong dictionary ?

Ahh sorry wrong thread haha, yeah these need to be internal my bad :D

spouliot · 2017-06-30T18:45:57Z

src/coreml.cs

+		// From MLMultiArray (NSNumberDataAccess) Category
+
+		[Export ("objectAtIndexedSubscript:")]
+		NSNumber GetObjectAtIndexedSubscript (nint idx);


GetObject ?

It this a duplicate of either Shape[] or Strides[] ?

To be honest, I cannot determine it from the documentation

spouliot · 2017-06-30T18:46:02Z

src/coreml.cs

+		NSNumber GetObjectAtIndexedSubscript (nint idx);
+
+		[Export ("objectForKeyedSubscript:")]
+		NSNumber GetObjectForKeyedSubscript (NSNumber [] key);


dalexsoto · 2017-06-30T19:11:27Z

@spouliot done :)

monojenkins · 2017-06-30T19:15:46Z

Build failure

monojenkins · 2017-06-30T19:25:46Z

Build failure

monojenkins · 2017-06-30T20:57:40Z

Build failure

dalexsoto · 2017-06-30T21:11:37Z

unrelated failure

Net.Http.NSUrlSessionHandler) -> https://bugzilla.xamarin.com/show_bug.cgi?id=57762

@timrisi Are you ok with merging this? Is there anything pending?

dnfclas added the cla-already-signed label Jun 29, 2017

timrisi suggested changes Jun 30, 2017

View reviewed changes

spouliot suggested changes Jun 30, 2017

View reviewed changes

dalexsoto and others added 5 commits June 30, 2017 13:02

[CoreML] Add Xcode 9 Beta 1 bindings

d8a5066

Do not C# 7 here... :'(

f41dc71

No C# 7 here either :'(

b44f29c

Apply feedback

5cb5a0d

[CoreML] Fix feedback and xtro

17334a7

dalexsoto force-pushed the coremlb1 branch from aea85d4 to 17334a7 Compare June 30, 2017 18:35

spouliot suggested changes Jun 30, 2017

View reviewed changes

[CoreML] Add more feedback

2982f93

Fix build.... What the heck was I thinking, always rebuild your stuff...

4352abd

spouliot approved these changes Jun 30, 2017

View reviewed changes

timrisi approved these changes Jun 30, 2017

View reviewed changes

dalexsoto merged commit f34b61d into xamarin:xcode9 Jun 30, 2017

dalexsoto deleted the coremlb1 branch June 30, 2017 22:35

[CoreML] Add Xcode 9 Beta 1 bindings #2275

[CoreML] Add Xcode 9 Beta 1 bindings #2275

Conversation

dalexsoto commented Jun 29, 2017

monojenkins commented Jun 29, 2017

monojenkins commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 30, 2017

dalexsoto commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 30, 2017

dalexsoto commented Jun 30, 2017

spouliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexsoto commented Jun 30, 2017

monojenkins commented Jun 30, 2017

monojenkins commented Jun 30, 2017

monojenkins commented Jun 30, 2017

dalexsoto commented Jun 30, 2017