-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-4859: [GLib] Add garrow_numeric_array_mean() #3889
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
kou
left a comment
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.
Thanks!
Could you check my comments?
c_glib/arrow-glib/basic-array.cpp
Outdated
| auto arrow_numeric_scalar = std::dynamic_pointer_cast<ScalarType>(mean_datum.scalar()); | ||
| return arrow_numeric_scalar->value; | ||
| } else { | ||
| return 0; |
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.
Could you use 0.0 for gdouble?
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.
I've fixed it.
c_glib/test/test-numeric-array.rb
Outdated
|
|
||
| def test_mean | ||
| array = build_double_array([1.1, 2.2, nil]) | ||
| assert_equal(1.6500000000000001, array.mean) |
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.
assert_equal isn't suitable for floating point number. Because floating point number may have an error.
Could you use assert_in_delta https://www.rubydoc.info/gems/test-unit/Test/Unit/Assertions#assert_in_delta-instance_method instead?
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.
Thank you for pointing out. I've fixed it.
c_glib/arrow-glib/basic-array.cpp
Outdated
| auto arrow_numeric_scalar = std::dynamic_pointer_cast<ScalarType>(mean_datum.scalar()); | ||
| return arrow_numeric_scalar->value; | ||
| } else { | ||
| return 0.0; |
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.
Should it be NaN?
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.
If we use NaN, std::numeric_limits<double>::signaling_NaN() https://en.cppreference.com/w/cpp/types/numeric_limits/signaling_NaN will be better.
But I don't think this is required. Because we indicate whether error is occurred by error argument. Users should check error instead of checking return value for error check.
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.
If @shiro615 changes this before I merge, I'll accept the change. :-)
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.
Users can check error argument. I want to use 0.0.
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.
OK.
kou
left a comment
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.
+1
I'll merge this when test is passed.
|
CI failure is unrelated. I'll merge this. https://travis-ci.org/apache/arrow/jobs/506259901 is Plasma related failure: @pcmoritz @robertnishihara Could you fix this Plasma failure? https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/23071125/job/s8iwxmf9hddh3q07 will be fixed by #3905. |
No description provided.