-
Notifications
You must be signed in to change notification settings - Fork 610
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
Update documentation of random.uniform to reflect data type conversion behavior #3013
Conversation
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.
Styling detail, otherwise ok.
This argument is mutually exclusive with ``values``.)code", | ||
This argument is mutually exclusive with ``values``. | ||
|
||
Warning: When specifying an integer type as ``dtype``, the generated numbers can go outside |
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.
Should we use the .. warning::
and .. note::
boxes from rst?
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.
Done
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.
Shouldn't we clamp. I think this is the last thing I would expect TBH.
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.
There are one more Warning and Note.
Signed-off-by: Joaquin Anton <janton@nvidia.com>
72059e7
to
d31a5dc
Compare
!build |
CI MESSAGE: [2432816]: BUILD STARTED |
|
||
Warning: When specifying an integer type as ``dtype``, the generated numbers can go outside |
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.
As above.
|
||
.. warning:: | ||
When specifying an integer type as ``dtype``, the generated numbers can go outside | ||
the specified range, due to rounding. |
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.
the generated numbers can go outside the specified range
Can't we clamp? It is a bit unexpected.
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.
So the RNG work like that, you generate the number in whatever is the natural type for the distribution, in this case float. Then if the user request the output data type to be a different type, we just ConvertSat(...). It's equivalent to doing fn.cast( fn.random.uniform(...), dtype=...)
, the numbers when converted to an integer data type, can go outside of the float range that was specified for the distribution. If the user needs a continuous distribution, it doesn't make much sense to request dtype to be an integer data type, I'd say.
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.
Make sense.
This argument is mutually exclusive with ``values``.)code", | ||
This argument is mutually exclusive with ``values``. | ||
|
||
Warning: When specifying an integer type as ``dtype``, the generated numbers can go outside |
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.
Warning -> .. warning::?
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.
Done
dali/operators/random/rng_base.cc
Outdated
R"code(Output data type.)code", nullptr); | ||
R"code(Output data type. | ||
|
||
Note: The generated numbers are converted to the output data type, rounding and clamping if necessary. |
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.
.. note::?
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.
Done
Signed-off-by: Joaquin Anton <janton@nvidia.com>
!build |
CI MESSAGE: [2432989]: BUILD STARTED |
CI MESSAGE: [2432989]: BUILD PASSED |
Signed-off-by: Joaquin Anton janton@nvidia.com
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
Documentation was extended
random.Uniform
NA
NA
NA
JIRA TASK: [NA]