You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2020/03/03 02:45:06 UTC
[GitHub] [incubator-datasketches-cpp] AlexanderSaydakov opened a new pull
request #98: frequent items weight type as a template parameter
AlexanderSaydakov opened a new pull request #98: frequent items weight type as a template parameter
URL: https://github.com/apache/incubator-datasketches-cpp/pull/98
This will allow changing the type for weights from uint64_t. For instance, something smaller such as uint32_t or float.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [incubator-datasketches-cpp] jmalkin commented on a change in pull
request #98: frequent items weight type as a template parameter
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #98: frequent items weight type as a template parameter
URL: https://github.com/apache/incubator-datasketches-cpp/pull/98#discussion_r388640866
##########
File path: fi/include/frequent_items_sketch_impl.hpp
##########
@@ -445,19 +446,35 @@ void frequent_items_sketch<T, W, H, E, S, A>::to_stream(std::ostream& os, bool p
}
}
+// version for integral signed type
template<typename T, typename W, typename H, typename E, typename S, typename A>
-template<typename WW, typename std::enable_if<std::is_signed<WW>::value, int>::type>
+template<typename WW, typename std::enable_if<std::is_integral<WW>::value && std::is_signed<WW>::value, int>::type>
void frequent_items_sketch<T, W, H, E, S, A>::check_weight(WW weight) {
if (weight < 0) {
throw std::invalid_argument("weight must be non-negative");
}
}
-// no-op for unsigned types
+// version for integral unsigned type - no-op
template<typename T, typename W, typename H, typename E, typename S, typename A>
-template<typename WW, typename std::enable_if<std::is_unsigned<WW>::value, int>::type>
+template<typename WW, typename std::enable_if<std::is_integral<WW>::value && std::is_unsigned<WW>::value, int>::type>
void frequent_items_sketch<T, W, H, E, S, A>::check_weight(WW weight) {}
+// version for floating point type
+template<typename T, typename W, typename H, typename E, typename S, typename A>
+template<typename WW, typename std::enable_if<std::is_floating_point<WW>::value, int>::type>
+void frequent_items_sketch<T, W, H, E, S, A>::check_weight(WW weight) {
+ if (weight < 0) {
+ throw std::invalid_argument("weight must be non-negative");
+ }
+ if (std::isnan(weight)) {
+ throw std::invalid_argument("weight must be a valid number");
+ }
+ if (weight > std::numeric_limits<WW>::max()) {
Review comment:
there's actually std::isinf() if you want to be more direct but i'm ok with this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [incubator-datasketches-cpp] jmalkin commented on a change in pull
request #98: frequent items weight type as a template parameter
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #98: frequent items weight type as a template parameter
URL: https://github.com/apache/incubator-datasketches-cpp/pull/98#discussion_r388466869
##########
File path: fi/include/frequent_items_sketch_impl.hpp
##########
@@ -440,6 +445,19 @@ void frequent_items_sketch<T, H, E, S, A>::to_stream(std::ostream& os, bool prin
}
}
+template<typename T, typename W, typename H, typename E, typename S, typename A>
+template<typename WW, typename std::enable_if<std::is_signed<WW>::value, int>::type>
Review comment:
i believe floats and doubles need additional checks for NaN and infinity
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [incubator-datasketches-cpp] AlexanderSaydakov merged pull request
#98: frequent items weight type as a template parameter
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov merged pull request #98: frequent items weight type as a template parameter
URL: https://github.com/apache/incubator-datasketches-cpp/pull/98
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org