add progress bar to FastICA #4234
Conversation
|
The code is based on Advanced usage of progress bar . Comments are greatly appreciated. |
src/shogun/converter/ica/FastICA.cpp
Outdated
| float64_t lim = tol+1; | ||
| while (lim > tol && iter < max_iter) | ||
| for (auto i : progress( | ||
| range(0, max_iter), "PROGRESS: ", UTF8, [&] { return lim > tol; })) |
There was a problem hiding this comment.
Minor, for consistency, indent and break lines like in the doc reference you've cited.
There was a problem hiding this comment.
Thanks @iglesias . The style has been modified.
|
Sorry that I did not mention it earlier, I have noticed only now the number of commits. Can you squash the 4 into 1? Thanks! |
src/shogun/converter/ica/FastICA.cpp
Outdated
| @@ -130,7 +130,11 @@ CFeatures* CFastICA::apply(CFeatures* features) | |||
|
|
|||
| float64_t lim = tol+1; | |||
| for (auto i : progress( | |||
| range(0, max_iter), "PROGRESS: ", UTF8, [&] { return lim > tol; })) | |||
| range(0, max_iter), | |||
| "PROGRESS: ", | |||
There was a problem hiding this comment.
Put this line and the two below at the same indentation than range, like in the doc we were referring to.
|
Thanks @iglesias I have changed the code style and squashed all the commits into one. |
sunalbert
force-pushed
the
feature/add_pd_for_fastICA
branch
from
049dfd7
to
e215825
Compare
|
@iglesias we can now squash with github, so people can just keep on adding commits to PRs (better to see trajectory, history, and preserving discussions (at least for small changes that make sense to group into a single commit) |
src/shogun/converter/ica/FastICA.cpp
Outdated
| while (lim > tol && iter < max_iter) | ||
| for (auto i : progress( | ||
| range(0, max_iter), | ||
| "PROGRESS: ", |
There was a problem hiding this comment.
@geektoni this seems a bit verbose. Isnt there a shorter version?
There was a problem hiding this comment.
Sorry! I just only realise this is the advanced way of using it
There was a problem hiding this comment.
@geektoni Could we have a way to pass lambda functions that does not require to pass the string and the encoding?
I.e. something as compact as the for (auto i: progress(range(0, 10))) but with a custom stopping condition?
There was a problem hiding this comment.
Sure! That's totally feasible. I should be able to make a patch for it later today.
There was a problem hiding this comment.
@sunalbert could you rebase and use the new function?
There was a problem hiding this comment.
Of course, you are welcome!
sunalbert
force-pushed
the
feature/add_pd_for_fastICA
branch
from
e215825
to
745e46f
Compare
modify code style fix style error automatically finetune the code style modify the code style again
sunalbert
force-pushed
the
feature/add_pd_for_fastICA
branch
from
745e46f
to
84ef012
Compare
|
Hi, all, sorry to disturb you guys again. I can not reproduce the problem in the local version. Any tips to fix the issues? |
|
All good the error is not caused by you |
add progress bar to FastICA algorithm.