Skip to content

renaming uint128 to RowId #1858

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 12, 2018
Merged

renaming uint128 to RowId #1858

merged 7 commits into from
Dec 12, 2018

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Dec 11, 2018

Fixes #1534 by renaming Uint128 to RowId

@sfilipi sfilipi self-assigned this Dec 11, 2018
@sfilipi sfilipi added the API Issues pertaining the friendly API label Dec 11, 2018
@sfilipi sfilipi added this to the 1218 milestone Dec 11, 2018
eerhardt
eerhardt reviewed Dec 11, 2018
View reviewed changes
src/Microsoft.ML.Core/Data/UInt128.cs
@@ -11,14 +11,14 @@ namespace Microsoft.ML.Runtime.Data
/// <summary>
/// A sixteen-byte unsigned integer.
/// </summary>
public readonly struct UInt128 : IComparable<UInt128>, IEquatable<UInt128>
public readonly struct RowId : IComparable<RowId>, IEquatable<RowId>
Copy link
Member

@eerhardt eerhardt Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

Can you rename the file as well? #Resolved

eerhardt
eerhardt reviewed Dec 11, 2018
View reviewed changes
src/Microsoft.ML.Core/Data/UInt128.cs Outdated
@@ -11,14 +11,14 @@ namespace Microsoft.ML.Runtime.Data
/// <summary>
/// A sixteen-byte unsigned integer.
Copy link
Member

@eerhardt eerhardt Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

Can you update this xml summary comment? This isn't really an "integer" anymore. #Resolved

Ivanidzo4ka
Ivanidzo4ka reviewed Dec 11, 2018
View reviewed changes
src/Microsoft.ML.Core/Data/UInt128.cs Outdated
{
// The low order bits. Corresponds to H1 in the Murmur algorithms.
public readonly ulong Lo;
// The high order bits. Corresponds to H2 in the Murmur algorithms.
public readonly ulong Hi;

public UInt128(ulong lo, ulong hi)
public RowId(ulong lo, ulong hi)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

public RowId(ulong lo, ulong hi) [](start = 8, length = 32)

Need proper summary.
I would probably renamed to lowerWord and highWord. #Resolved

Copy link
Member

@eerhardt eerhardt Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

If we rename the parameters of the constructor, we should rename the public fields as well. The names should be insync. #Resolved

Ivanidzo4ka
Ivanidzo4ka reviewed Dec 11, 2018
View reviewed changes
src/Microsoft.ML.Core/Data/UInt128.cs
@@ -167,7 +167,7 @@ private static void FinalMix(ref ulong h1, ref ulong h2, int len)
/// that were all zeros, except for the last bit which is one.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public UInt128 Fork()
public RowId Fork()
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

Fork [](start = 21, length = 4)

can be internal, right? #Resolved

Copy link
Member

@eerhardt eerhardt Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

I will be moving this and any other types that IDataView needs out into a separate assembly, which will not have InternalsVisibleTo on it. So let's keep whatever things we need as public.

Alternatively, the Lo and Hi fields are public, so we could make these methods static extension methods that were only in ML.NET. (And those extension methods would be internal.) #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look on usage, only thing which we use outside is Combine method.
Part of me wants just delete all this unused code, but since it was written by someone, maybe it can be useful someday.
So I would prefer just hide it behind internal/private.

Just in case I'm talking about Hashing region.

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

Copy link
Contributor

@TomFinley TomFinley Dec 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Part of me wants just delete all this unused code,

Please do not remove this code. It is needed for definite purposes as explained in our code documentation. Refer to ICursor.md if the point of its usage is unclear.

eerhardt
eerhardt reviewed Dec 11, 2018
View reviewed changes
src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.cs Outdated
@@ -194,7 +194,7 @@ public static ulong ToULong(this byte[] buffer, ref int position)
return a;
}

// UInt128
// RowId

public static MD5Hash ToUInt128(this byte[] buffer, ref int position)
Copy link
Member

@eerhardt eerhardt Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

Does this method need to be renamed? #Resolved

@eerhardt
Copy link
Member

eerhardt commented Dec 11, 2018 •
edited by sfilipi
Loading 

    public static unsafe MD5Hash[] ToUInt128Array(this byte[] buffer, ref int position)

Same here - Uint128 #Resolved

Refers to: src/Microsoft.ML.FastTree/Utils/ToByteArrayExtensions.cs:569 in d335fc6. [](commit_id = d335fc6, deletion_comment = False)

eerhardt
eerhardt reviewed Dec 11, 2018
View reviewed changes
src/Microsoft.ML.Core/Data/RowId.cs Outdated
// The high order bits. Corresponds to H2 in the Murmur algorithms.
public readonly ulong Hi;
///<summary>The low order bits. Corresponds to H1 in the Murmur algorithms.</summary>
public readonly ulong LowerWord;
Copy link
Member

@eerhardt eerhardt Dec 11, 2018 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

I should have caught this in Ivan's suggestion, but the word Word shouldn't be used here. There is a certain connotation that "WORD" means 2 bytes - especially for Windows C++ developers.

My preference for these names is simply Low and High. #Resolved

eerhardt
eerhardt approved these changes Dec 11, 2018
View reviewed changes
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.

Looks good. Thanks Senja.

Ivanidzo4ka
Ivanidzo4ka approved these changes Dec 11, 2018
View reviewed changes
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:

TomFinley
TomFinley approved these changes Dec 12, 2018
View reviewed changes
Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thank you @sfilipi !

TomFinley
TomFinley suggested changes Dec 12, 2018
View reviewed changes
Copy link
Contributor

@TomFinley TomFinley left a comment •
edited by sfilipi
Loading

Choose a reason for hiding this comment

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

Hi @sfilipi , it is essential that Fork, Combine, and Next be public members on RowId. Please refer to our documentation on this subject to see why. Without them being public, it will be difficult or impossible in some easy to imagine situations to write a correct implementation of row ID getters. Thanks.#Resolved

TomFinley
TomFinley approved these changes Dec 12, 2018
View reviewed changes
Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thank you @sfilipi !

@sfilipi sfilipi merged commit a326dda into dotnet:master Dec 12, 2018
@sfilipi sfilipi deleted the renameUint128 branch December 12, 2018 19:12
@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.
Reviewers

@Ivanidzo4ka Ivanidzo4ka Ivanidzo4ka approved these changes

@eerhardt eerhardt eerhardt approved these changes

@TomFinley TomFinley TomFinley approved these changes

@najeeb-kazmi najeeb-kazmi Awaiting requested review from najeeb-kazmi

@wschin wschin Awaiting requested review from wschin

@glebuk glebuk Awaiting requested review from glebuk

Assignees

@sfilipi sfilipi

Labels
API Issues pertaining the friendly API
Projects
None yet
Milestone
1218
Development

Successfully merging this pull request may close these issues.
4 participants
@sfilipi @eerhardt @Ivanidzo4ka @TomFinley