-
Notifications
You must be signed in to change notification settings - Fork 1.9k
replace e.g. with for example #1016
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
Conversation
@@ -337,13 +337,13 @@ aside (which we can hardly help), we expect the models to be the same. | |||
When you create a loader you have the option of specifying not only *one* data | |||
input, but any number of data input files, including zero. But there's also a | |||
more general principle at work here with zero files: when deserializing a data | |||
loader from a data model with an `IMultiStreamSource` with `Count == 0` (e.g., | |||
loader from a data model with an `IMultiStreamSource` with `Count == 0` (for example, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little unfortunate that the linebreaking got all screwed up due to the longer lines, but I guess maybe we can opportunistically fix it later? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomFinley this is usually a personal choice. Some people choose to cut the text that way, no?
Are we going to have a separate PR for i.e.? ;) Edit: I was joking but I see from the PR description but this was actually the plan. |
@@ -79,7 +79,7 @@ public static ColumnInfo CreateFromIndex(ISchema schema, int index) | |||
/// individually configured). Also, by being a one-to-many mapping, it is a way for learners that can consume | |||
/// multiple features columns to consume that information. | |||
/// | |||
/// This class has convenience fields for several common column roles (se.g., <see cref="Feature"/>, <see | |||
/// This class has convenience fields for several common column roles (sfor example, <see cref="Feature"/>, <see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling (propagated from previous) #Closed
@@ -22,7 +22,7 @@ Or from within Visual Studio's NuGet package manager. | |||
|
|||
This initial release contains core ML.NET components for enabling machine learning pipelines: | |||
|
|||
* ML Data Structures (e.g. `IDataView`, `LearningPipeline`) | |||
* ML Data Structures (for example, `IDataView`, `LearningPipeline`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example [](start = 22, length = 11)
Live documentation seems fine, I guess. This however I'm not sure about. These are release notes that have already been released. They're already out. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to revert in those cases @TomFinley?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// (c) Use FailedPrecondition if the client should not retry until | ||
/// the system state has been explicitly fixed. E.g., if an "rmdir" | ||
/// the system state has been explicitly fixed. For example, if an "rmdir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: extra space after period. #Closed
@@ -25,7 +25,7 @@ public partial class ApiScenariosTests | |||
/// *) The tree ensemble learners, I should be able to inspect the trees. | |||
/// *) The LDA transform, I should be able to inspect the topics. | |||
/// I view it as essential from a usability perspective that this be discoverable to someone without | |||
/// having to read documentation.E.g.: if I have var lda = new LdaTransform().Fit(data)(I don't insist on that | |||
/// having to read documentation.For example,: if I have var lda = new LdaTransform().Fit(data)(I don't insist on that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.F [](start = 41, length = 2)
Spacing please... and a spare comma. #Closed
@@ -13,7 +13,7 @@ public partial class ApiScenariosTests | |||
{ | |||
/// <summary> | |||
/// Visibility: It should, possibly through the debugger, be not such a pain to actually | |||
/// see what is happening to your data when you apply this or that transform. E.g.: if I | |||
/// see what is happening to your data when you apply this or that transform. For example,: if I |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra comma #Closed
@@ -33,7 +33,7 @@ private TOut GetValue<TOut>(Dictionary<string, object> keyValues, string key) | |||
/// *) The tree ensemble learners, I should be able to inspect the trees. | |||
/// *) The LDA transform, I should be able to inspect the topics. | |||
/// I view it as essential from a usability perspective that this be discoverable to someone without | |||
/// having to read documentation.E.g.: if I have var lda = new LdaTransform().Fit(data)(I don't insist on that | |||
/// having to read documentation.For example,: if I have var lda = new LdaTransform().Fit(data)(I don't insist on that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space & extra comma #Closed
@@ -13,7 +13,7 @@ public partial class ApiScenariosTests | |||
{ | |||
/// <summary> | |||
/// Visibility: It should, possibly through the debugger, be not such a pain to actually | |||
/// see what is happening to your data when you apply this or that transform. E.g.: if I | |||
/// see what is happening to your data when you apply this or that transform. For example,: if I |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra comma #Closed
@@ -1403,7 +1403,7 @@ private sealed class ImplVec<T> : ColumnCache<VBuffer<T>> | |||
// For a given row [r], elements at [r] and [r+1] specify the inclusive | |||
// and exclusive range of values for the two big arrays. In the case | |||
// of indices, if that range is empty, then the corresponding stored | |||
// vector is dense. E.g.: row 5 would have its vector's values stored | |||
// vector is dense. For example,: row 5 would have its vector's values stored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e,: [](start = 46, length = 3)
Comma #Closed
@@ -1403,7 +1403,7 @@ private sealed class ImplVec<T> : ColumnCache<VBuffer<T>> | |||
// For a given row [r], elements at [r] and [r+1] specify the inclusive | |||
// and exclusive range of values for the two big arrays. In the case | |||
// of indices, if that range is empty, then the corresponding stored | |||
// vector is dense. E.g.: row 5 would have its vector's values stored | |||
// vector is dense. For example,: row 5 would have its vector's values stored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra comma / colon #Closed
@@ -513,7 +513,7 @@ public sealed class SuggestedSweepsParser | |||
/// Generic parameter parser. Currently hand-hacked to auto-detect type. | |||
/// | |||
/// Generic form: Name:Values | |||
/// e.g.: lr:0.05-0.4 | |||
/// for example,: lr:0.05-0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e,: [](start = 22, length = 3)
Comma #Closed
@@ -513,7 +513,7 @@ public sealed class SuggestedSweepsParser | |||
/// Generic parameter parser. Currently hand-hacked to auto-detect type. | |||
/// | |||
/// Generic form: Name:Values | |||
/// e.g.: lr:0.05-0.4 | |||
/// for example,: lr:0.05-0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra comma #Closed
@@ -25,7 +25,7 @@ public partial class ApiScenariosTests | |||
/// *) The tree ensemble learners, I should be able to inspect the trees. | |||
/// *) The LDA transform, I should be able to inspect the topics. | |||
/// I view it as essential from a usability perspective that this be discoverable to someone without | |||
/// having to read documentation.E.g.: if I have var lda = new LdaTransform().Fit(data)(I don't insist on that | |||
/// having to read documentation.For example,: if I have var lda = new LdaTransform().Fit(data)(I don't insist on that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra comma / colon #Closed
@@ -1776,7 +1776,7 @@ internal enum TFCode : uint | |||
Aborted = 10, | |||
|
|||
/// <summary> | |||
/// Operation tried to iterate past the valid input range. E.g., seeking or | |||
/// Operation tried to iterate past the valid input range. For example, seeking or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space after period #Closed
Feedback and merge conflicts resolved. I know it's a funny PR @TomFinley but given this are making into the product and documentation, I think we should follow the style guide. Decided to separate the i.e. just to not overwhelm you with a giant PR. |
@@ -14,7 +14,7 @@ namespace Microsoft.ML.StaticPipe.Runtime | |||
/// <summary> | |||
/// A schema shape with names corresponding to a type parameter in one of the typed variants | |||
/// of the data pipeline structures. Instances of this class tend to be bundled with the statically | |||
/// typed variants of the dynamic structures (e.g., <see cref="DataView{TShape}"/> and so forth), | |||
/// typed variants of the dynamic structures (for example, <see cref="DataView{TTupleShape}"/> and so forth), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was renaming TShape
to TTupleShape
intentional? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. I probably did a wrong analysis when resolving conflicts. The only changes should be spaces and e.g. -> for example #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mairaw thanks for doing the work! |
Thanks everyone for their feedback. I think this should be ready then! |
I'm going to close/re-open to restart the CI tests. Error looks to be:
|
I've noticed in some of the API docs for ML.NET that I was browsing that there is a heavy use of e.g. which is against our style guide rules (https://docs.microsoft.com/en-us/style-guide/a-z-word-list-term-collections/e/eg). So replaced it across the board on the repo.
Once this is merged, I can create a separate PR for i.e. which also shouldn't be used.