Skip to content
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 wrong behaviour of SGIO::progress #3694

Merged
merged 1 commit into from Mar 15, 2017

Conversation

geektoni
Copy link
Contributor

See #3509.

SG_PROGRESS current behaviour:

previous_behaviour

SG_PROGRESS patch behaviour:

new_behaviour

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

Seems ok to me, some minor code style issues should be fixed

Somebody else should also comment

@@ -181,8 +181,18 @@ void SGIO::progress(
v=CMath::clamp(v,1e-5,100.0);
last_progress = v-1e-6;

if ((v!=100.0) && (runtime - last_progress_time<0.5))
if ((v!=100.0) && (runtime - last_progress_time<0.5)) {
Copy link
Member

Choose a reason for hiding this comment

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

style: we do { in a new line


// This is made to display correctly the percentage
// if the algorithm execution is too fast
if (current_val >= max_val-1){
Copy link
Member

Choose a reason for hiding this comment

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

style

@geektoni
Copy link
Contributor Author

Style fixed.

@karlnapf
Copy link
Member

Do we actually have a unit test for this thing? Just to ensure nothing crashes?

@geektoni
Copy link
Contributor Author

As far as I know, the entire SGIO has no unit tests, which means no, there is no unit test to ensure the correctness of the code. Shall I spend some time creating one just to be sure it works as before?

@karlnapf
Copy link
Member

I think a test that at least executes the stuff you added would be good. Correctness is tricky as it just prints out. Not sure it is worth the hassle to check this. But making sure it doesnt crash on corner cases would be useful definitely :)

@karlnapf
Copy link
Member

This one is fine to merge from my side

@vigsterkr vigsterkr merged commit 2599ce5 into shogun-toolbox:develop Mar 15, 2017
karasikov pushed a commit to karasikov/shogun that referenced this pull request Apr 15, 2017
Fix wrong behaviour of SGIO::progress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants