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

[DOC] Fix mathematical representation on interval (range) #27450

Merged
merged 3 commits into from
Nov 21, 2017

Conversation

aeroastro
Copy link
Contributor

@aeroastro aeroastro commented Nov 20, 2017

I have fixed mathematical representation on intervals of histogram.

  • Fix representation on range.
  • Add additional description on the range.
  • Remove ambiguity on example description.

cf.
https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@aeroastro aeroastro changed the title [DOC] Fix mathematical representation on interval (range) of histogram [DOC] Fix mathematical representation on interval (range) Nov 20, 2017
@s1monw
Copy link
Contributor

s1monw commented Nov 20, 2017

@elasticmachine ok to test

@@ -14,7 +14,8 @@ To make this more formal, here is the rounding function that is used:
bucket_key = Math.floor((value - offset) / interval) * interval + offset
--------------------------------------------------

The `interval` must be a positive decimal, while the `offset` must be a decimal in `[0, interval[`.
The `interval` must be a positive decimal, while the `offset` must be a decimal in `[0, interval)`
(a decimal greater than or euqal to `0` and less than `interval`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "equal" not "euqal"


This can be best illustrated with an example. If there are 10 documents with values ranging from 5 to 14, using interval `10` will result in
two buckets with 5 documents each. If an additional offset `5` is used, there will be only one single bucket [5-14] containing all the 10
This can be best illustrated with an example. If there are 10 documents with integer values ranging from 5 to 14, using interval `10` will result in
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is true even if the values are not integers.

@@ -266,10 +267,10 @@ The above will sort the buckets based on the avg rating among the promoted produ
==== Offset

By default the bucket keys start with 0 and then continue in even spaced steps of `interval`, e.g. if the interval is 10 the first buckets
(assuming there is data inside them) will be [0 - 9], [10-19], [20-29]. The bucket boundaries can be shifted by using the `offset` option.
(assuming there is data inside them) will be [0, 10), [10, 20), [20, 30). The bucket boundaries can be shifted by using the `offset` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, can you format these ranges as code (i.e. within backticks) please?

This can be best illustrated with an example. If there are 10 documents with values ranging from 5 to 14, using interval `10` will result in
two buckets with 5 documents each. If an additional offset `5` is used, there will be only one single bucket [5-14] containing all the 10
This can be best illustrated with an example. If there are 10 documents with integer values ranging from 5 to 14, using interval `10` will result in
two buckets with 5 documents each. If an additional offset `5` is used, there will be only one single bucket [5, 15) containing all the 10
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, can you format this range as code (i.e. within backticks) please?

@DaveCTurner
Copy link
Contributor

Thanks @aeroastro - as a mathematician I like this change. I left a handful of comments.

@elasticmachine please test again but with feeling this time.

* Fix typo on equal
* Remove redundant integer
* code format
@aeroastro
Copy link
Contributor Author

@DaveCTurner
Thank you for your quick and detailed review!
I have fixed the document.

@DaveCTurner
Copy link
Contributor

Ok, I suspect the CI build is failing because your branch is based off 40fe786 from back in December. Could you merge the current master into your branch and we'll try again?

@aeroastro
Copy link
Contributor Author

@DaveCTurner
Thank you for your advice. I've merged the current master, and now the CI build is successful.

@DaveCTurner DaveCTurner assigned colings86 and unassigned jpountz Nov 20, 2017
@DaveCTurner
Copy link
Contributor

LGTM, but it's not an area I'm 100% familiar with so asking @colings86 to glance over it too. Thanks again @aeroastro.

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

@DaveCTurner DaveCTurner merged commit eed8d1a into elastic:master Nov 21, 2017
@DaveCTurner
Copy link
Contributor

Thanks for the contribution @aeroastro!

DaveCTurner pushed a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 21, 2017
jasontedor added a commit to olcbean/elasticsearch that referenced this pull request Nov 21, 2017
* master: (41 commits)
  [Test] Fix AggregationsTests#testFromXContentWithRandomFields
  [DOC] Fix mathematical representation on interval (range) (elastic#27450)
  Update version check for CCS optional remote clusters
  Bump BWC version to 6.1.0 for elastic#27469
  Adapt rest test BWC version after backport
  Fix dynamic mapping update generation. (elastic#27467)
  Use the primary_term field to identify parent documents (elastic#27469)
  Move composite aggregation to core (elastic#27474)
  Fix test BWC version after backport
  Protect shard splitting from illegal target shards (elastic#27468)
  Cross Cluster Search: make remote clusters optional (elastic#27182)
  [Docs] Fix broken bulleted lists (elastic#27470)
  Move resync request serialization assertion
  Fix resync request serialization
  Fix issue where pages aren't released (elastic#27459)
  Add YAML REST tests for filters bucket agg (elastic#27128)
  Remove tcp profile from low level nio channel (elastic#27441)
  [TEST] Fix `GeoShapeQueryTests#testPointsOnly` failure
  Transition transport apis to use void listeners (elastic#27440)
  AwaitsFix GeoShapeQueryTests#testPointsOnly elastic#27454
  ...
@aeroastro
Copy link
Contributor Author

Thank you 🐱

@aeroastro aeroastro deleted the feature/doc-fix branch November 22, 2017 01:35
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants