Skip to content

[BEAM-3527] Adding a comment for ZERO element of DistributionResult.#4538

Merged
kennknowles merged 1 commit intoapache:masterfrom
pabloem:comment-for-zero
Feb 6, 2018
Merged

[BEAM-3527] Adding a comment for ZERO element of DistributionResult.#4538
kennknowles merged 1 commit intoapache:masterfrom
pabloem:comment-for-zero

Conversation

@pabloem
Copy link
Copy Markdown
Member

@pabloem pabloem commented Jan 30, 2018

I've decided to not rename the ZERO, because I get the feeling that IDENTITY might be even less clear (though I'm open to having my mind changed:) ). I've only added a comment to clarify its purpose.
cc: @bjchambers
cc: @clairemcginty

@robertwb
Copy link
Copy Markdown
Contributor

+1 to changing your mind. Another alternative would be EMPTY. I though ZERO was the distribution with nothing but zeros in it.

@clairemcginty
Copy link
Copy Markdown
Contributor

@robertwb EMPTY could be good as it matches what GaugeResult has -- although, still not a perfect analogue as EmptyGaugeResult just returns a value of -1.

@pabloem
Copy link
Copy Markdown
Member Author

pabloem commented Jan 30, 2018

hmm how about we go with IDENTITY_ELEMENT ?

@clairemcginty
Copy link
Copy Markdown
Contributor

Sounds good to me!

@pabloem pabloem force-pushed the comment-for-zero branch 2 times, most recently from 86d17e0 to 816e5da Compare January 31, 2018 00:06
@pabloem
Copy link
Copy Markdown
Member Author

pabloem commented Feb 1, 2018

r: @kennknowles
getting another reviewer

@kennknowles
Copy link
Copy Markdown
Member

SGTM LGTM

@kennknowles kennknowles merged commit a67262f into apache:master Feb 6, 2018
@pabloem pabloem deleted the comment-for-zero branch February 6, 2018 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants