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

Add PRange class to replace old SG_PROGRESS (WIP) #3745

Merged
merged 5 commits into from Jun 1, 2017

Conversation

Projects
None yet
4 participants
@geektoni
Contributor

geektoni commented Mar 23, 2017

This is a prototype for the new progress bar that will (hopefully) replace SG_PROGRESS. This is not a full-fledged solution, it is just to show the idea and to get some feedback about it, so be warned ;)

As @lisitsyn suggested back here #3693, I developed a wrapper that takes a range() object and an SGIO instance. Every time the iterator is incremented, the wrapper should print a message on screen.

More specifically:

  • PRange is the wrapper class;
  • PRange<T>::PIterator is the iterator that will increment the given Range<T>::Iterator;
  • ProgressPrinter is the actual class that will implement the progress bar logic;

The syntax should be something like this:

for (auto i : progress(range(1, 10), io)) 
{
 // Do stuff
}

I've thought about more ways to specify a progress object to customize its behaviour (I was inspired by this https://github.com/tqdm/tqdm):

for (auto i : progress( range(1, 10), io, "Custom_prefix: ", ASCII) )
{
 // Do stuff
}

In this example, the progress call sets the ASCII chars mode (print the progress bar using only ASCII chars) and a custom prefix that will be printed before the real progress bar.

I am aware that there are many things that can be improved and refactored (for example the execution speed), but, as I said above, this is just an "exploratory" patch.


Other changes:

  • Remove HAVE_CXX11 guards from range.h since shogun needs C++11 now;
  • Correct Range::Iterator::operator++(int) behaviour (it was updating a local variable);
@lisitsyn

This comment has been minimized.

Member

lisitsyn commented Mar 23, 2017

Amazing!

namespace shogun {
/** Possible print modes */
enum SG_PRG_MODE {

This comment has been minimized.

@karlnapf

karlnapf Mar 23, 2017

Member

minor style comment. Put { to new line

@karlnapf

Like it a lot as well.

Can you paste a use case with calling code and produced output?

@karlnapf

This comment has been minimized.

Member

karlnapf commented Mar 23, 2017

And any ideas how to integrate this with tmdq :D

@lisitsyn

This comment has been minimized.

Member

lisitsyn commented Mar 23, 2017

@karlnapf why integrate it to python lib??

@karlnapf

This comment has been minimized.

Member

karlnapf commented Mar 23, 2017

In other words: How does it look like when shogun is called from python. Can we maybe use it with the tmdq if available, so in ipython notebook we get similar kind of optics....not very important

@geektoni

This comment has been minimized.

Contributor

geektoni commented Mar 23, 2017

Possible use case:

#include <shogun/base/init.h>
#include <shogun/base/progress.h>
#include <shogun/io/SGIO.h>

int main() {
 
 init_shogun_with_defaults();

 SGIO io;
 for (auto i : progress(range(1, 100), io, "PROGRESS: ", UTF8))
 {
  // Do computations
 }
 exit_shogun();

return 0;
}

(Hoped) Result:
optimised

@lisitsyn

This comment has been minimized.

Member

lisitsyn commented Mar 23, 2017

@karlnapf I think that's different scope. If we use this new progress(...) thing all around we can integrate it to anything.

@karlnapf

This comment has been minimized.

Member

karlnapf commented Mar 23, 2017

Sure differente scope. But I got excited about it anyways.... to cite @OXPHOS : this is georgeous

* @param range range used
*/
template <typename T>
inline PRange<T> progress(Range<T> range, const SGIO & io)

This comment has been minimized.

@lisitsyn

lisitsyn Mar 24, 2017

Member

Let's add one more progress(Range<T> range) that uses global SGIO to simplify things. It's initialized with init_shogun_with_defaults

This comment has been minimized.

@geektoni

geektoni Mar 24, 2017

Contributor

Updated as requested.

@lisitsyn

This comment has been minimized.

Member

lisitsyn commented Mar 24, 2017

👍 to merge. @karlnapf what about you?

@geektoni

This comment has been minimized.

Contributor

geektoni commented Mar 24, 2017

@lisitsyn merge? This is only a work in progress version, there is no actual progress bar code (I mean, there is only the wrapper for the range method). Shouldn't we wait until it's finished?

@lisitsyn

This comment has been minimized.

Member

lisitsyn commented Mar 25, 2017

@geektoni hahah I was too excited! Sorry

@karlnapf

This comment has been minimized.

Member

karlnapf commented Mar 25, 2017

Lets tidy it until @geektoni is pleased ;)

Add PRange class skeleton to replace old SG_PROGRESS.
Other changes:
* Remove HAVE_CXX11 guards from range.h since Shogun needs C++11 now;
* Correct Range<T>::Iterator::operator++(int) behaviour (it was updating a local variable);

@geektoni geektoni changed the base branch from develop to feature/progress May 30, 2017

[ProgressBar] Add progress bar code on top of its "skeleton".
* Add print_progress() and print_end() code;
* The progress bar can now adjust its size depending on the screen
  dimension (should work also on windows);
* The progress bar must be enabled by the user by using SGIO enable_progress();
@geektoni

This comment has been minimized.

Contributor

geektoni commented May 30, 2017

I've added the method's implementations. It should behave even better than the old SG_PROGRESS now ;)

// Check for terminal dimension. This is for provide
// a minimal resize functionality.
set_screen_size();

This comment has been minimized.

@geektoni

geektoni May 30, 2017

Contributor

This method is run each time we print the progress bar to account for the terminal size, which can change (for instance, if the user resizes or expands the window). However, I'm not so sure this is worth it (since it doesn't work so well). Should I remove this line? @vigsterkr

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

could you elaborate the part "doesn't work so well"?

This comment has been minimized.

@geektoni

geektoni May 31, 2017

Contributor

The method gets the size of the terminal, which will be used to calculate how much space we will use to print the progress bar. If the user resizes/expands the window, this method will correctly get the new dimensions, but there are cases in which the progress bar will be still printed in a wrong way (even if the program "knows" that the terminal size has changed).

(m_columns_num - 50 - m_prefix.length()) * 0.9;
// TODO: this guy here brokes testing
// REQUIRE(

This comment has been minimized.

@geektoni

geektoni May 30, 2017

Contributor

I was planning to throw an error if the user enables the progress bar, but the terminal size is too small to print it. This is currently disabled because it makes the unit tests crash. Do you think it is something we need? So that I can work on re-enable it without destroying the test suite. @vigsterkr

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

mmm i think we can remove this as is.... i mean what is the terminal size when this will be a problem?

This comment has been minimized.

@geektoni

geektoni May 31, 2017

Contributor

It is calculated dynamically. There is a formula above which will calculate how much space we will use to print the progress bar. Sometimes the result can be 0 (or less), and that is a case in which we cannot print the progress bar.

@geektoni

This comment has been minimized.

Contributor

geektoni commented May 30, 2017

@vigsterkr Travis fails because of the clang-format script. Probably, it doesn't like the new base branch that I set for this PR (since it works on my machine). All the tests pass, though.

@@ -0,0 +1,434 @@
/*

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

pls copy here the bsd license

// Check for terminal dimension. This is for provide
// a minimal resize functionality.
set_screen_size();

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

could you elaborate the part "doesn't work so well"?

(m_columns_num - 50 - m_prefix.length()) * 0.9;
// TODO: this guy here brokes testing
// REQUIRE(

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

mmm i think we can remove this as is.... i mean what is the terminal size when this will be a problem?

(m_max_value - m_min_value + 1);
// Set up chunk size
size_chunk = difference / (double_t)progress_bar_space;

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

float64_t or?

@@ -1,128 +1,127 @@
#ifndef __SG_RANGE_H__

This comment has been minimized.

@vigsterkr

vigsterkr May 31, 2017

Member

same as above. license is missing :)

geektoni added some commits May 30, 2017

[ProgressBar] Add progress bar unit tests
Other changes:
* Disable exception which is thrown when the screen size is not
  enough (since it brokes the tests).
* Add BSD license.
@vigsterkr

This comment has been minimized.

@vigsterkr vigsterkr merged commit 8de270b into shogun-toolbox:feature/progress Jun 1, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
default DEV build done.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment