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

more transform => transformer renaming #1590

Merged
merged 6 commits into from
Nov 12, 2018
Merged

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Nov 9, 2018

Fixes part of #1318

@sfilipi sfilipi added the API Issues pertaining the friendly API label Nov 9, 2018
@sfilipi sfilipi self-assigned this Nov 9, 2018
Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

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

:shipit:

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Nov 12, 2018

    public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)

nit: unrelated, but should be internal or private same for all other signature methods. #Pending


Refers to: src/Microsoft.ML.Transforms/NAReplaceTransform.cs:464 in ca16fa1. [](commit_id = ca16fa1, deletion_comment = False)

@@ -249,7 +249,7 @@ private string GetBuildPrefix()
#endif
}

[Fact]
[Fact(Skip = "Execute this test if you want to regenerate the core_manifest and core_ep_list files")]
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Nov 12, 2018

Choose a reason for hiding this comment

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

Skip = "Execute this test if you want to regenerate the core_manifest and core_ep_list files") [](start = 14, length = 94)

https://media.tenor.com/images/f6f91f0f5bd4d733e1fb9dfee1c37580/tenor.gif #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally AM!


In reply to: 232741497 [](ancestors = 232741497)

[assembly: LoadableClass(NADropTransform.Summary, typeof(NADropTransform), null, typeof(SignatureLoadDataTransform),
NADropTransform.FriendlyName, NADropTransform.LoaderSignature)]
[assembly: LoadableClass(MissingValueDroppingTransformer.Summary, typeof(MissingValueDroppingTransformer), null, typeof(SignatureLoadDataTransform),
MissingValueDroppingTransformer.FriendlyName, MissingValueDroppingTransformer.LoaderSignature)]

namespace Microsoft.ML.Runtime.Data
{
/// <include file='doc.xml' path='doc/members/member[@name="NADrop"]'/>
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Nov 12, 2018

Choose a reason for hiding this comment

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

NADrop [](start = 64, length = 6)

nit: these names can potentially backfire, but I'm don't know what our plans for them, and documentation in general. #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

documentation needs to be scrubbed from the term transform, and adapted to transformer/estimator. Didn't want to bundle those changes together with the renames, because the renames have to finish on this release.


In reply to: 232742005 [](ancestors = 232742005)

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:

@sfilipi
Copy link
Member Author

sfilipi commented Nov 12, 2018

    public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)

Agree. I will leave it since that can be a bug/change on its own, and it is currently unrelated to this bug.


In reply to: 437957613 [](ancestors = 437957613)


Refers to: src/Microsoft.ML.Transforms/NAReplaceTransform.cs:464 in ca16fa1. [](commit_id = ca16fa1, deletion_comment = False)

@sfilipi sfilipi merged commit 28fb3bb into dotnet:master Nov 12, 2018
@sfilipi sfilipi deleted the transformers2 branch November 12, 2018 18:41
@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
API Issues pertaining the friendly API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants