-
Notifications
You must be signed in to change notification settings - Fork 329
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
Don't reset trackers when sampling is finished #1862
Conversation
// Don't exceed 'max' samples in total. | ||
assert(trackers[0].get_size() <= m_params.m_max_samples); | ||
const size_t remaining_samples = m_params.m_max_samples - trackers[0].get_size(); | ||
if (remaining_samples == 0) | ||
break; | ||
|
||
trackers[0].reset_variation(); |
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.
Yes, that looks correct actually.
I would go even further: I would move the other termination checks just after the check on the remaining number of samples:
// Stop if the variation criterion is met.
if (trackers[0].get_variation() <= m_params.m_max_variation &&
trackers[1].get_variation() <= m_params.m_max_variation &&
trackers[2].get_variation() <= m_params.m_max_variation)
break;
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 you do so, sampling will stop on the first step. Just have to check the size at the same time.
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.
Oh you're right, sorry. Maybe it's better to leave the test where it was then. Sorry for the troubles!
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.
Ok no problem :)
@@ -241,7 +241,7 @@ namespace | |||
trackers[1].insert(shading_result.m_main[1]); | |||
trackers[2].insert(shading_result.m_main[2]); | |||
} | |||
|
|||
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.
Remove trailing spaces.
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.
That's what happen when you edit the code directly on github :(
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.
Yeah, not recommended especially since it's easy to introduce mistakes that will break compilation :)
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.
Thanks!
This is not a fix for #1858 but a fix for a mistake in the adaptive sampler. The trackers were resat before writing them in an image when diagnostic is enabled.