Skip to content

Make CpuMath not depending on ML.Core again #1724

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

Merged
merged 12 commits into from
Nov 27, 2018

Conversation

wschin
Copy link
Member

@wschin wschin commented Nov 26, 2018

Fixes #1688.

CpuMath project now includes Contract.cs in ML.Core. The file Contract.cs contains some if-else macros for distinguishing the part for general ML.NET (namespace: Microsoft.ML) from the part for CpuMath(namespace: Microsoft.ML.Runtime.Internal.CpuMath.Core). The BestFriendAttribute in ML.Core (namespace: Microsoft.ML) and CpuMath (namespace: Microsoft.ML.Runtime.Internal.CpuMath.Core) follow the same pattern for the same reason. BestFriendAnalyzer is also modified to support the two variants of BestFriend attributes.

Besides, we move PublicKey into another file, PublicKey.cs, to avoid including AssemblyInfo.cs twice in CpuMath project.

@eerhardt
Copy link
Member

eerhardt commented Nov 27, 2018

// Licensed to the .NET Foundation under one or more agreements.

(nit) I think it makes more sense for this file to live outside of the Properties folder, and next to the BestFriendAttribute.cs file. They are in the same "namespace". #Resolved


Refers to: src/Microsoft.ML.Core/Properties/PublicKey.cs:1 in 7f374c6. [](commit_id = 7f374c6, deletion_comment = False)

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Just the question about if using #if PRIVATE_CONTRACTS as the name still makes sense, and a minor question about the old namespace.

Other than those 2 things, this looks good.

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@wschin wschin self-assigned this Nov 27, 2018
@wschin wschin added the bug Something isn't working label Nov 27, 2018
@wschin wschin merged commit 870cf7e into dotnet:master Nov 27, 2018
@wschin wschin deleted the isolate-cpumath branch November 27, 2018 20:25
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants