-
Notifications
You must be signed in to change notification settings - Fork 622
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
Fix uniform generator operator #2352
Conversation
!build |
CI MESSAGE: [1695888]: BUILD STARTED |
CI MESSAGE: [1695888]: BUILD FAILED |
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.
IMHO this introduces unwanted behaviour in the test and we shouldn't go this way. I'd be better to fix the operator
If the range is [a,b] we should say so in the documentation. |
- for some reason uniform_real_distribution returns [a, b] instead of [a, b) so ask the generator to draw samples until they are < b - see http://open-std.org/JTC1/SC22/WG21/docs/lwg-active.html#2524 Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
98da885
to
96384b2
Compare
!build |
dali/operators/random/uniform.cc
Outdated
while (sample_data[k] >= range_[1]) { | ||
sample_data[k] = dist(rng_); | ||
} |
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.
while (sample_data[k] >= range_[1]) { | |
sample_data[k] = dist(rng_); | |
} | |
do { | |
sample_data[k] = dist(rng_); | |
} while (sample_data[k] >= range_[1]); // Due to GCC and LLVM bug |
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/uniform.cc
Outdated
if (range_[0] >= range_[1]) { | ||
for (int i = 0; i < batch_size_; ++i) { | ||
sample_data[k] = range_[0]; | ||
} | ||
return; | ||
} |
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 suggest adding argument verification in Uniform
constructor, sth like:
if (discrete_mode_) {
set_ = spec.GetRepeatedArgument<float>("values");
DALI_ENFORCE(!set_.empty(), "`values` argument cannot be empty");
} else {
range_ = spec.GetRepeatedArgument<float>("range");
DALI_ENFORCE(range_.size() == 2, "`range` argument shall contain precisely 2 values");
DALI_ENFORCE(range_[0] < range_[1],
"Invalid range. It shall be left-closed [a, b), where a < b");
}
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
CI MESSAGE: [1696987]: BUILD STARTED |
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
!build |
CI MESSAGE: [1697021]: BUILD STARTED |
dali/operators/random/uniform.cc
Outdated
if (range_[0] >= range_[1]) { | ||
for (int i = 0; i < batch_size_; ++i) { | ||
sample_data[k] = range_[0]; | ||
} | ||
return; | ||
} |
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's no becessary if we enfornce that the range is not empty.
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: Janusz Lisiecki <jlisiecki@nvidia.com>
!build |
CI MESSAGE: [1697032]: BUILD STARTED |
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
!build |
CI MESSAGE: [1697074]: BUILD STARTED |
CI MESSAGE: [1697074]: BUILD FAILED |
CI MESSAGE: [1697074]: BUILD PASSED |
Signed-off-by: Janusz Lisiecki jlisiecki@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 []
so ask the generator to draw samples until they are < b
uniform operator
NA
CI
NA
JIRA TASK: [NA]