"polish activation" #9740
"polish activation" #9740
Conversation
panyx0718
left a comment
panyx0718
left a comment
There was a problem hiding this comment.
LG overall
It would be great if we have a single source of truth of the in-place activation op list.
There was a problem hiding this comment.
can this flatten been avoided?
There was a problem hiding this comment.
Can we have this in C++
static char* inplace_acts = ["sigmoid", "exp", ...]
bool IsInPlaceAct(const string& n) {
return n in inplace_acts;
}
Inplace() { IsInPlaceAct("exp") }
in Python, we expose IsInPlaceAct() as core._is_in_place_act()
dzhwinter
force-pushed
the
memory/activation
branch
from
e8901e2 to
81e6e59
Compare
dzhwinter
force-pushed
the
memory/activation
branch
from
8524460 to
877e2ae
Compare
paddle/fluid/pybind/pybind.cc
Outdated
There was a problem hiding this comment.
You can do that as a follow up, it seems the python style is is_in_place().
There was a problem hiding this comment.
This function bind c++ and Python, I thought that we should follow the c++ style where it been defined.
| return | ||
| self.check_grad(['X'], 'Out', max_relative_error=0.007) | ||
| # the gradient on floor, ceil, round is undefined. | ||
| # we return zero as gradient, but the numpy return nan |
There was a problem hiding this comment.
Something to consider in the future
returning None or Nan might be preferred since it avoids constructing a large zero tensor, saving some time and space.
Just curious, why it works before this PR?
| # pybind USE_OP | ||
| if (${pybind_flag} EQUAL 0) | ||
| # NOTE(*): activation use macro to regist the kernels, set use_op manually. | ||
| if(${TARGET} STREQUAL "activation") |
There was a problem hiding this comment.
Hopefully, we can remove this hard-coded "activation" and "relu" strings in the cmake files the future.
Add a TODO to clean up?
There was a problem hiding this comment.
Yep. The CMakeLists in operators need a clean up, will file a PR do this later.
3 participants
fix #9416