haojin2 commented on a change in pull request #14900: Fix warning / static
function in header.
URL: https://github.com/apache/incubator-mxnet/pull/14900#discussion_r286820805
##########
File path: src/operator/nn/dropout.cc
##########
@@ -26,6 +26,32 @@
#include "./dropout-inl.h"
#include "../operator_common.h"
+#include "mxnet/op_attr_types.h"
+
+namespace {
+
+using namespace mxnet;
+using namespace mxnet::op;
Review comment:
Just a quick note, I believe that grouping the 2 specifc PRs in my last
comment, which are essentially both doing exactly the same thing (changing from
using mx_test_utils.list_gpus to mx.context.num_gpus), would lead to easier
bisection in the future than having 2 different PRs which will be merged far
apart on the timeline. And, even if you combine those 2, it would become a
+30/-30-ish PR, which I would not consider as a big and difficult-to-review
one. To be fair, I'm not trying to blame you for not grouping those PRs, simply
suggesting a better way of performing similar fixes in the future.
On the other hand, I believe that while your time is precious, so is other
people's. The reason to have a code review process is to make sure that every
PR is merged to help the community to be a better one as a whole, otherwise why
don't we automatically merge a PR if it does not break the CI?
I do understand that everyone tend to defend their own preferred way of
doing certain things (I defend my code/docs/PRs from time to time too), but
we're a community and you're also part of it (I'm not sure why you identify
yourself as an "external contributor", I believe you have been in the community
for quite a while, maybe you could elaborate a bit?), which means that we want
to be considerate to others during our development. Organizing and documenting
one's code/commits/PR in a fashion that's more logical and easier to
understand/debug/re-use/extend should be a duty for everyone in the community,
because we're not working alone on a private project. If those values do not
align with what you believe at this moment, [Apache
Guidelines](https://www.apache.org/foundation/policies/conduct.html#specific-guidelines)
would be a good read.
I think this PR LGTM now, I'll approve it and it could be merged as long as
@anirudh2290 has no more comments (there have been several new changes since
his last approval). Thanks for your contribution.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
