Add SGIO::progress unit tests and enhance SGIO::progress #3705

geektoni · 2017-03-15T16:10:14Z

Unit tests for #3694 plus little changes to SGIO::progress to make unit tests pass.

karlnapf

I like having unit tested progress bars :)

Some minor things required, then good to merge

karlnapf · 2017-03-16T11:00:19Z

src/shogun/io/SGIO.cpp

@@ -165,7 +165,16 @@ void SGIO::progress(
 	float64_t v=-1, estimate=0, total_estimate=0 ;

 	if (max_val-min_val>0.0)
+	{


we usually dont do extra {} for single lines

karlnapf · 2017-03-16T11:00:36Z

src/shogun/io/SGIO.cpp

+	}
+	else
+	{
+		/* Wrong range, exit */


superflous comment

karlnapf · 2017-03-16T11:01:08Z

src/shogun/io/SGIO.cpp

@@ -189,6 +196,7 @@ void SGIO::progress(
 			if (current_val >= max_val-1)
 			{
 				v = 100;
+				last_progress=v-1e-6;


if you do that, why not initialise it with 0 above rather than -1?

Are you referring to last_progress or v? If you're referring to last_progress I think of -1 as a value that means not initialized and I used it since there are cases in which last_progress is not set (for example when max_val-min_val<=0). Obviously, this is completely subjective. Using 0 instead of -1 should not make any difference. :)

Yeah it doesnt, just a tiny bit cleaner....your call

Core developer's will is law ;). Jokes aside, I will change it to 0. As I said, it is the same 👍

Hehe ;)
We gotta make everything as good as we possibly can -- chances that somebody touches code again are pretty low, so better get it correct (and neat) in the first place.

Hey are you in IRC at some point, I remember you asked about dependencies ....

Yeah, I am still curious about them ;). Regarding IRC, I could be online tomorrow morning, will I find you @karlnapf?

karlnapf · 2017-03-16T11:01:52Z

src/shogun/io/SGIO.h

@@ -288,6 +288,13 @@ class SGIO
 			return location_info;
 		}

+		/** get last progress


@return last time progress was registered

I.e. needs doxygen and a human understandable description (this is API doc)

karlnapf · 2017-03-16T11:02:32Z

tests/unit/io/SGIO_unittest.cc

+
+TEST(SGIOTest, progress_correct_bounds)
+{
+	/* Positive values */


pls split positive and negative into separate tests. tests should be as small as possible

karlnapf · 2017-03-16T11:02:43Z

tests/unit/io/SGIO_unittest.cc

+	EXPECT_EQ(tmp2.get_last_progress(), (float64_t)-1);
+
+	/* Negative values */
+	SGIO tmp;


same here, split

karlnapf · 2017-03-16T14:13:51Z

src/shogun/io/SGIO.h

@@ -288,6 +288,15 @@ class SGIO
 			return location_info;
 		}

+		/** get last_progress


Sorry for being picky, but these things are never fixed otherwise.
You can just remove the text here, and only have the @ in a single line

karlnapf · 2017-03-16T14:37:52Z

src/shogun/io/SGIO.h

@@ -288,6 +288,12 @@ class SGIO
 			return location_info;
 		}

+		/** @return last progress as percentage */


karlnapf · 2017-03-16T14:38:07Z

Ill merge once CI is green

karlnapf · 2017-03-17T09:58:47Z

tests/unit/io/SGIO_unittest.cc

+{
+	SGIO tmp;
+	tmp.enable_progress();
+	for (int i=0; i<100; i++) {


minor, new line for {

karlnapf

Sorry there is some things wrong with the tests that need fix. Didnt see that earlier

karlnapf · 2017-03-17T09:59:13Z

tests/unit/io/SGIO_unittest.cc

+{
+	SGIO tmp2;
+	tmp2.enable_progress();
+	for (int i=-100; i>0; i++) {


this loop is never executed

argh noob fail

karlnapf · 2017-03-17T10:00:22Z

tests/unit/io/SGIO_unittest.cc

+	SGIO tmp2;
+	tmp2.enable_progress();
+	tmp2.progress(0, 100, 1);
+	EXPECT_EQ(tmp2.get_last_progress(), (float64_t)0);


(float64_t)0 can be just 0.0 But in fact that is not even necessary.
What you want to use here is EXPECT_NEAR which compares floating point numbers, where you can set the epsilon to machine precision or so

Can I use EXPECT_FLOAT_EQ? Seems to have the same behaviour as EXPECT_NEAR and also its meaning is more straightforward.

Sure! Didn't know about that guy actually

karlnapf · 2017-03-17T10:00:45Z

tests/unit/io/SGIO_unittest.cc

+	SGIO tmp;
+	tmp.enable_progress();
+	tmp.progress(0, -1, -2);
+	EXPECT_EQ(tmp.get_last_progress(), (float64_t)0);


same here, sorry I didnt spot those earler.
Cannot check floats for equality....it is not portable

Same question as above

Add a getter to retrieve the last_progress (for testing purpose).

karlnapf · 2017-03-17T17:54:26Z

I'm heikoS You might need to try a few times, I'm more online under the week than on weekend

On Fri, 17 Mar 2017 at 17:26, Giovanni De Toni ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/shogun/io/SGIO.cpp <#3705 (comment)> : > @@ -189,6 +196,7 @@ void SGIO::progress( if (current_val >= max_val-1) { v = 100; + last_progress=v-1e-6; Yeah, I am still curious about them ;). Regarding IRC, I could be online tomorrow morning, will I find you @karlnapf <https://github.com/karlnapf>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3705 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAqqv4CO-OIPkQ-OZmRy2fjjSJpx3KTlks5rmsJEgaJpZM4MeKpA> .

-- Sent from my phone

geektoni · 2017-03-23T16:38:08Z

@karlnapf any update on this one?

karlnapf · 2017-03-23T17:34:10Z

Nice patch, thanks!

karlnapf · 2017-03-23T17:34:19Z

@vigsterkr @lisitsyn pls merge

Add SGIO::progress unit tests and enhance SGIO::progress

karlnapf requested changes Mar 16, 2017

View reviewed changes

geektoni force-pushed the patch-13 branch 2 times, most recently from bebcb3d to b451da9 Compare March 16, 2017 13:31

karlnapf reviewed Mar 16, 2017

View reviewed changes

geektoni force-pushed the patch-13 branch from b451da9 to b128f0e Compare March 16, 2017 14:29

karlnapf approved these changes Mar 16, 2017

View reviewed changes

geektoni force-pushed the patch-13 branch from b128f0e to c5a23fe Compare March 16, 2017 14:40

karlnapf reviewed Mar 17, 2017

View reviewed changes

karlnapf requested changes Mar 17, 2017

View reviewed changes

geektoni force-pushed the patch-13 branch from c5a23fe to ac072f3 Compare March 17, 2017 13:54

Add SGIO::progress unit tests and enhance SGIO::progress.

36e0794

Add a getter to retrieve the last_progress (for testing purpose).

geektoni force-pushed the patch-13 branch from ac072f3 to 36e0794 Compare March 17, 2017 17:20

vigsterkr merged commit 17d36a6 into shogun-toolbox:develop Mar 23, 2017

karasikov pushed a commit to karasikov/shogun that referenced this pull request Apr 15, 2017

Merge pull request shogun-toolbox#3705 from geektoni/patch-13

9ce5282

Add SGIO::progress unit tests and enhance SGIO::progress

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SGIO::progress unit tests and enhance SGIO::progress #3705

Add SGIO::progress unit tests and enhance SGIO::progress #3705

geektoni commented Mar 15, 2017

karlnapf left a comment

karlnapf Mar 16, 2017

karlnapf Mar 16, 2017

karlnapf Mar 16, 2017

geektoni Mar 16, 2017 •

edited

karlnapf Mar 16, 2017

geektoni Mar 16, 2017 •

edited

karlnapf Mar 17, 2017

geektoni Mar 17, 2017

karlnapf Mar 16, 2017

karlnapf Mar 16, 2017

karlnapf Mar 16, 2017

karlnapf Mar 16, 2017

karlnapf Mar 16, 2017

karlnapf commented Mar 16, 2017

karlnapf Mar 17, 2017 •

edited

karlnapf left a comment

karlnapf Mar 17, 2017

geektoni Mar 17, 2017

karlnapf Mar 17, 2017

geektoni Mar 17, 2017

karlnapf Mar 17, 2017

karlnapf Mar 17, 2017

geektoni Mar 17, 2017

karlnapf Mar 17, 2017

karlnapf commented Mar 17, 2017 via email

geektoni commented Mar 23, 2017

karlnapf commented Mar 23, 2017

karlnapf commented Mar 23, 2017

Add SGIO::progress unit tests and enhance SGIO::progress #3705

Add SGIO::progress unit tests and enhance SGIO::progress #3705

Conversation

geektoni commented Mar 15, 2017

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geektoni Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geektoni Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf commented Mar 16, 2017

karlnapf Mar 17, 2017 • edited

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf commented Mar 17, 2017 via email

geektoni commented Mar 23, 2017

karlnapf commented Mar 23, 2017

karlnapf commented Mar 23, 2017

geektoni Mar 16, 2017 •

edited

geektoni Mar 16, 2017 •

edited

karlnapf Mar 17, 2017 •

edited