Skip to content

Conversation

imaginary-person
Copy link
Contributor

@imaginary-person imaginary-person commented Dec 29, 2020

PROBLEM DESCRIPTION:
GitHub issue 46391 suggests that compiler warnings pertaining to floating-point value does not fit in required integral type might cause some confusion.

These compiler-warnings arise during compilation of the templated function uniform_int(). The warnings are misleading because they arise from the way the compiler compiles templated functions, but the if-else statements in the function obviate the possibilities that the warnings describe. So, the purpose of a fix would only be to fix the compiler warnings, and not to fix any sort of a bug.

FIX DESCRIPTION:
[EDITED, after inputs from @malfet]: In the function uniform_int(), the if-else conditions pertaining to types double & float can be removed, and then an overloaded specialized function can be added for floating-point types. The current version of the function can be specialized to not have its return type as a floating point type.

An unrelated observation is that the if-else condition pertaining to the type double (line 57 in the original code) was redundant, as line 61 in the original code covered it (std::is_floating_point<T>::value would also have been true for the type double).

Fixes #46391

PROBLEM DESCRIPTION:
GitHub issue 46391 suggests that compiler warnings pertaining to "floating-point value does not fit in required integral type" might cause some confusion.

These compiler-warnings arise during compilation of the templated function uniform_int(). The warnings are misleading because they arise from the way the compiler compiles templated functions, but the if-else statements in the function obviate the possibilities that the warnings describe. So, the purpose of a fix would only be to fix the compiler warnings, and not to fix any sort of a bug.


FIX DESCRIPTION:
In the function uniform_int(), the if-else conditions pertaining to types double & float can be removed,
and then uniform_int() can be overloaded for both double & float.
Since template specialization for functions cannot be partial, we would have to add four overloaded versions of uniform_int(typename T, typename V) pertaining to 
type T being either double or float, and type V being either uint32_t or uint64_t.

An unrelated observation is that the if-else condition pertaining to the type 'double' (line 57 in the original code) was redundant, as line 61 in the original code covered it (std::is_floating_point<T>::value would also have been true for the type 'double').
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 29, 2020

💊 CI failures summary and remediations

As of commit 64c7d6d (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 10 times.

@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #49914 (64c7d6d) into master (c619892) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #49914   +/-   ##
=======================================
  Coverage   80.70%   80.70%           
=======================================
  Files        1895     1895           
  Lines      205496   205495    -1     
=======================================
+ Hits       165840   165845    +5     
+ Misses      39656    39650    -6     

@mruberry mruberry requested a review from malfet December 29, 2020 17:33
@mruberry mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Dec 29, 2020
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix!

Comment on lines 70 to 102
* An overloaded transformation function for `torch.Tensor.random_()`, when used without specifying `from` and `to`,
* added to fix compiler warnings reported in GitHub issue 46391.
* Here, <T, V> is <float, uint32_t>
*/
template <>
C10_HOST_DEVICE inline float uniform_int(uint32_t val) {
return static_cast<float>(val % static_cast<uint64_t>((1ULL << std::numeric_limits<float>::digits) + 1));
}

/**
* An overloaded transformation function for `torch.Tensor.random_()`, when used without specifying `from` and `to`,
* added to fix compiler warnings reported in GitHub issue 46391.
* Here, <T, V> is <float, uint64_t>
*/
template <>
C10_HOST_DEVICE inline float uniform_int(uint64_t val) {
return static_cast<float>(val % static_cast<uint64_t>((1ULL << std::numeric_limits<float>::digits) + 1));
}

/**
* An overloaded transformation function for `torch.Tensor.random_()`, when used without specifying `from` and `to`,
* added to fix compiler warnings reported in GitHub issue 46391.
* Here, <T, V> is <double, uint32_t>
*/
template <>
C10_HOST_DEVICE inline double uniform_int(uint32_t val) {
return static_cast<double>(val % static_cast<uint64_t>((1ULL << std::numeric_limits<double>::digits) + 1));
}

/**
* An overloaded transformation function for `torch.Tensor.random_()`, when used without specifying `from` and `to`,
* added to fix compiler warnings reported in GitHub issue 46391.
* Here, <T, V> is <double, uint64_t>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits:

  • Can you fold 4 comment blocks into 4?
  • This implies that those functions are only called for unsigned integer type. Is it possible to add a compile time check that those templates could not be instantiated with signed integral type?
    P.S. Also, please explain why something like the following wouldn't work (instead of 4 explicit full specializations):
template<typename T, typename V>
C10_HOST_DEVICE inline std::enable_if<std::is_floating_point<T>::value, T>::type uniform_int(V val) {
    return static_cast<T>(val % static_cast<uint64_t>((1ULL << std::numeric_limits<T>::digits) + 1));
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your prompt review, and for this learning opportunity!

I had never used enable_if before, so I agree that your suggestion is way better than using full specializations.

As for the compile-time check for disallowing instantiation with a signed integral type, could you please suggest what should the behavior be in that case? Please advise if an assertion would be appropriate. Thanks!

Based on @malfet's advice, used std::enable_if instead of using four full specializations
I had introduced an empty line by mistake. Removed it
As per malfet's advice, overloaded uniform_int() for floating-point types & non-floating-point types to prevent compiler warnings reported via GitHub issue 46391.

Didn't change any code from the last commit in the forked repo, but only edited a comment, so as to re-run tests automatically on Circle CI, and check if any reported build failures on Circle CI were due to this commit, although it seems highly unlikely.
@imaginary-person
Copy link
Contributor Author

CI failures summary and remediations

As of commit 64c7d6d (more details on the Dr. CI page):

  • 1/1 failures possibly* introduced in this PR

    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

This comment was automatically generated by Dr. CI (expand for details).

pr/pytorch-linux-bionic-rocm3.10-py3.6 is actually failing due to another reason, as the failure-cause is same in some of the previous builds that didn't have this change.

Copy link
Contributor Author

@imaginary-person imaginary-person left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @malfet, based on your advice, I had revised the code to have one overloaded version of uniform_int(T, V) that corresponds to T being a floating point type. As for the original function, I ensured that T won't be a floating point type.

The sole failed check reported by Jenkins yesterday was unrelated to the change in this PR & seems to have been fixed now. Can you please check if this fix is acceptable? Thanks!

Snippets from the commit -

/**
 * A transformation function for `torch.Tensor.random_()`, when used without specifying `from` and `to`.
 * In order to prevent compiler warnings reported in GitHub issue 46391, T can't be float or double
 * in this overloaded version
 */
template <typename T, typename V>
C10_HOST_DEVICE inline typename std::enable_if<!(std::is_floating_point<T>::value), T>::type uniform_int(V val) {
/**
 * An overloaded transformation function for `torch.Tensor.random_()`, when used without specifying `from` and `to`,
 * added to fix compiler warnings reported in GitHub issue 46391. T is either float or double in this version.
 */
template<typename T, typename V>
C10_HOST_DEVICE inline typename std::enable_if<std::is_floating_point<T>::value, T>::type uniform_int(V val) {
  return static_cast<T>(val % static_cast<uint64_t>((1ULL << std::numeric_limits<T>::digits) + 1));
}

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@malfet merged this pull request in 7377bfb.

hwangdeyu pushed a commit to hwangdeyu/pytorch that referenced this pull request Jan 14, 2021
Summary:
**PROBLEM DESCRIPTION:**
GitHub issue 46391 suggests that compiler warnings pertaining to _floating-point value does not fit in required integral type_ might cause some confusion.

These compiler-warnings arise during compilation of the templated function `uniform_int()`. The warnings are misleading because they arise from the way the compiler compiles templated functions, but the if-else statements in the function obviate the possibilities that the warnings describe. So, the purpose of a fix would only be to fix the compiler warnings, and not to fix any sort of a bug.

**FIX DESCRIPTION:**
[EDITED, after inputs from malfet]: In the function `uniform_int()`, the if-else conditions pertaining to types `double` & `float` can be removed, and then an overloaded specialized function can be added for floating-point types. The current version of the function can be specialized to not have its return type as a floating point type.

An unrelated observation is that the if-else condition pertaining to the type `double` (line 57 in the original code) was redundant, as line 61 in the original code covered it (`std::is_floating_point<T>::value` would also have been true for the type `double`).

Fixes pytorch#46391

Pull Request resolved: pytorch#49914

Reviewed By: H-Huang

Differential Revision: D25808037

Pulled By: malfet

fbshipit-source-id: 3f94c4bca877f09720b0d6efa5e1788554aba074
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warnings during compiling: floating-point value does not fit in required integral type

5 participants