-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Feature/is nan #7068
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
Feature/is nan #7068
Conversation
Make them as usual names.
Make them as usual names.
Make them as usual names.
…dle into feature/enhance_dev_ctx_pool
| void operator()() const { | ||
| auto t = EigenVector<T>::Flatten(tensor_); | ||
| auto o = EigenScalar<bool>::From(*out_); | ||
| o.device(*ctx_.eigen_device()) = predicate_(t).any(); |
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.
We can add a reference comment on eigen any() for better understanding
https://eigen.tuxfamily.org/dox/classEigen_1_1DenseBase.html#abfbf4cb72dd577e62fbe035b1c53e695
| template <typename T> | ||
| auto operator()(const T& eigen_vec) const | ||
| -> decltype(std::declval<T>().isnan()) { | ||
| return eigen_vec.isnan(); |
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.
add a reference comment:
https://eigen.tuxfamily.org/dox/classEigen_1_1ArrayBase.html#aab10b156cb69206461728782dd01c37a
paddle/framework/tensor_util.cc
Outdated
| tmp.Resize({1}); | ||
| tmp.mutable_data<bool>(cpu); | ||
| platform::DeviceContextPool::Instance().Get(gpu)->Wait(); | ||
| CopyFrom(out, cpu, &tmp); |
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 do copy between CPU and GPU, we need to pass a DeviceContext
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.
However, the device context is a global variable. We can get DeviceContext by its place.
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 mean the CopyFrom interface is
Paddle/paddle/framework/tensor_util.h
Lines 33 to 34 in 1831176
| inline void CopyFrom(const Tensor& src, const platform::Place& dst_place, | |
| const platform::DeviceContext& ctx, Tensor* dst) { |
| #endif | ||
| } | ||
|
|
||
| TEST(IsNAN, CPU) { |
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.
Please add GPU unit tests at the same time.
| }; | ||
|
|
||
| template <typename Place> | ||
| struct DefaultDeviceContextType; |
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.
What is the DefaultDeviceContextType used for?
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.
It is used for DeviceCtxPool::GetByPlace() method. This method will return the casted DeviceContext by PlaceType.
In the future, we could add a library type to this method.
| } | ||
|
|
||
| // Returns true if a tensor contains NAN, i.e., Not A Number. | ||
| extern bool HasNAN(const framework::Tensor& tensor); |
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.
extern seems superfluous.
| @@ -0,0 +1 @@ | |||
| ./tensor_util.cc No newline at end of file | |||
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.
Why not move the code in tensor_util.cc to tensor_util.h. Or we can have a tensor_util_impl.h. It's a little strange to have a symbolic link.
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.
@QiJune Reyoung probably had an offline discussion with you already. But looks like .cu is necessary to make nvcc pass the compilation...
| ASSERT_TRUE(HasNAN(src)); | ||
| } | ||
|
|
||
| TEST(IsInf, CPU) { |
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.
IsInf --> HasInf
QiJune
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.
LGTM
This is a part of #7092 .