'Done' Button Implementation #2177

jepietryga · 2021-06-03T18:35:00Z

This pull request implements a 'Done' button, specifically intended for iterative reconstruction algorithms that reach a desirable level of completion prior reaching the set number of iterations. Operators that are "Doneable" will be able to use this button to early terminate processing, but unlike cancel, the data will still be useable by subsequent operators (such as "Square Root") without restarting the pipeline.

Most changes are necessary for this functionality, but Recon_SIRT.py was changed simply to take advantage of it.

This addresses issue #2162.

cjh1

A few inline comments, would be happy to talk in person about them.

cjh1 · 2021-08-16T00:49:00Z

tomviz/ProgressDialogManager.cxx

@@ -81,7 +81,23 @@ void ProgressDialogManager::operationStarted()
    layout->addWidget(progressBar);
  }
  layout->addWidget(progressWidget);
-  if (op->supportsCancelingMidTransform()) {
+  if (op->supportsDoneingMidTransform()) {


Can we come up with a better name I don't "Doneing" is a word :-) What about "early completion"

cjh1 · 2021-08-16T00:51:17Z

tomviz/operators/Operator.h

@@ -34,7 +34,8 @@ enum class OperatorState
  Complete,
  Canceled,
  Error,
-  Edit
+  Edit,
+  Done


Do we need a new state or can we just use Complete?

cjh1 · 2021-08-16T00:54:07Z

tomviz/operators/Operator.h

  bool isCanceled() { return m_state == OperatorState::Canceled; }
+  bool isDone()
+  {
+    return m_state == OperatorState::Done || m_state == OperatorState::Complete;


For the look of this, Complete could be used without need to add another state.

cjh1 · 2021-08-16T00:55:00Z

tomviz/operators/OperatorPython.cxx

@@ -144,6 +145,11 @@ OperatorPython::OperatorPython(DataSource* parentObject)
      qCritical() << "Unable to locate is_cancelable.";
    }

+    d->IsDoneableFunction = d->InternalModule.findFunction("is_doneable");


supports_early_completion?

cjh1 · 2021-08-16T00:59:04Z

tomviz/operators/OperatorPython.cxx

@@ -144,6 +145,11 @@ OperatorPython::OperatorPython(DataSource* parentObject)
      qCritical() << "Unable to locate is_cancelable.";
    }

+    d->IsDoneableFunction = d->InternalModule.findFunction("is_doneable");
+    if (!d->IsDoneableFunction.isValid()) {
+      qCritical() << "Unable to locate is_doneable.";


You probably want a return, as the rest of the function is not going to work without the python function.

cjh1 · 2021-08-18T19:41:28Z

@jepietryga Why was thirdparty/pybind11 changed?

jepietryga · 2021-08-18T19:52:36Z

@jepietryga Why was thirdparty/pybind11 changed?

It shouldn't have been, I didn't make any modifications to the thirdparty/pybind11 when I ran "git commit *".

I have been having some git issues, so it's possible I accidentally pulled the most recent pybind11 while I was trying to fix things?

It may be necessary for me to drop this current pull request and set-up a new one once these things are fixed.

Hovden · 2021-08-25T18:44:02Z

@jepietryga @cjh1 Is this one ready for merge?

cjh1 · 2021-08-25T18:49:04Z

I think there are still some flake8 and clang format issues?

jepietryga · 2021-08-25T19:26:42Z

I'll finalize this.

There are some flake8 issues that I can't fix too easily, so some advice would be appreciated:
Recon_DFT.py is a file I hadn't manipulated, but has one really long line due to deep nesting and readability.
Recon_TV_Minimization.py has a complexity issue in the class' transform function. I added two additional conditionals to break out of loops if something is early_completed, but this seems to have pushed it into flake8's notice.

The other flake issues and one clang issue should be quickly fixed.

EDIT: The two above listed issues are the only remaining format concerns from flake8, clang is 100% good now!

cjh1 · 2021-08-25T21:48:50Z

I'll finalize this.

There are some flake8 issues that I can't fix too easily, so some advice would be appreciated:
Recon_DFT.py is a file I hadn't manipulated, but has one really long line due to deep nesting and readability.
Recon_TV_Minimization.py has a complexity issue in the class' transform function. I added two additional conditionals to break out of loops if something is early_completed, but this seems to have pushed it into flake8's notice.

The other flake issues and one clang issue should be quickly fixed.

EDIT: The two above listed issues are the only remaining format concerns from flake8, clang is 100% good now!

We can add some #no qa comments for things that you haven't touched or don't want to tackle, but we want flake8 to be green otherwise other things will slip in.

cjh1

@jepietryga We have fixed up the CI issues on our end of the CI is now clean ( along with you clang-format and flake8 fixes, thank you. I have one more suggestion and you can take it or leave it. Seeing earlyCompleted typed out makes me thing that is should just be completed and CompletableOperator, others feel free to chime in. I think we are close, I just want to get the naming right as this will be part of the interface.

cjh1 · 2021-08-27T13:39:43Z

tomviz/operators/Operator.h

+
+  /// Distinction between this and isFinished is necessary to prevent cascading
+  /// errors
+  bool isEarlyCompleted() { return m_state == OperatorState::Complete; }


Shouldn't this just be isCompleted, more general and could be used else where.

cjh1 · 2021-08-27T13:40:53Z

tomviz/python/Recon_ART.py

@@ -58,9 +58,15 @@ def transform(self, dataset, Niter=1, Nupdates=0, beta=1.0):
        counter = 1
        for i in range(Niter):

+            if self.early_completed:


I think this should just be completed, if so we should make the change elsewhere as well.

cjh1 · 2021-08-27T13:43:18Z

tomviz/python/Recon_SIRT.py

@@ -4,7 +4,7 @@
 import time


-class ReconSirtOperator(tomviz.operators.CancelableOperator):
+class ReconSirtOperator(tomviz.operators.EarlyCompletableOperator):


Sorry to be a pain but seeing the names "written" down, may be this should just be CompletableOperator

jepietryga · 2021-08-27T15:38:51Z

In process of making the recommended changes! I think the recommended naming makes sense.

jepietryga · 2021-08-28T18:54:06Z

@cjh1 Code should be working! The things I checked were using a sample dataset, then trying ART, SIRT, and TV Minimization. Part of the way through a reconstruction, I clicked the "Ok" button, then did some data transforms to ensure it didn't try to redo the pipeline

tomviz/python/Recon_DFT.py

tomviz/python/tomviz/_internal.py

tomviz/python/Recon_TV_minimization.py

…tive operators to be prematurely finished but still have their data used in a pipeline. Signed-off-by: jepi <jacobpietryga@gmail.com> Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Signed-off-by: jepi <jacobpietryga@gmail.com> Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

…ators to be early cancelable Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Co-authored-by: Christian Clauss <cclauss@me.com> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

cjh1

@jepietryga LGTM, thanks for sticking with it!

jepietryga force-pushed the doneButton branch 2 times, most recently from e3cd470 to 4b9aa13 Compare June 4, 2021 15:43

jtschwar requested a review from cryos June 4, 2021 19:40

cjh1 reviewed Aug 16, 2021

View reviewed changes

jepietryga force-pushed the doneButton branch from 461601d to fee159a Compare August 18, 2021 17:56

jepietryga force-pushed the doneButton branch from 925fff2 to fee159a Compare August 18, 2021 21:16

cjh1 reviewed Aug 27, 2021

View reviewed changes

jepietryga force-pushed the doneButton branch from 2964e28 to 610cc03 Compare August 27, 2021 16:40

cclauss reviewed Sep 1, 2021

View reviewed changes

tomviz/python/Recon_DFT.py Outdated Show resolved Hide resolved

cclauss reviewed Sep 1, 2021

View reviewed changes

tomviz/python/tomviz/_internal.py Outdated Show resolved Hide resolved

cclauss reviewed Sep 1, 2021

View reviewed changes

tomviz/python/Recon_TV_minimization.py Outdated Show resolved Hide resolved

jepietryga and others added 11 commits September 1, 2021 09:53

Fixed formatting issues associated with clang-format and flake8

470885f

Signed-off-by: jepi <jacobpietryga@gmail.com> Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixing last clang-format issue

01bd6ef

Signed-off-by: jepi <jacobpietryga@gmail.com> Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixing last clang-format issue

91a82e7

Signed-off-by: jepi <jacobpietryga@gmail.com> Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixing last clang-format issue

1a2dffa

Signed-off-by: jepi <jacobpietryga@gmail.com> Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Chris' recommended updates for naming

ddd916a

Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixed to be "EarlyCancelable" Operator

a1999ae

Signed-off-by: Jacob Pietryga <jepi@umich.edu> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixed flake and clang issues, no thirdparty change, updated more oepr…

81df160

…ators to be early cancelable Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Further adjustment of flake8 and clang issues

8796f9c

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fix of line length flake issue

eb8b627

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

flake8 fix

b8c9143

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

jepietryga and others added 9 commits September 1, 2021 09:53

Added "# no qa" to account for a few intentional formatting issues

68c39b9

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

' # no qa' to '# noqa'

5c26443

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

' # noqa' for function header rather than conditionals

9f77c2f

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixed whitespace trailing on previous comment lines

14ae028

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Updating nomenclature: "EarlyComplete" -> "Complete"

f6ddd84

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Fixing 'clang' issues

1eb1b67

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Shortening Conditional

3d45c18

Co-authored-by: Christian Clauss <cclauss@me.com> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Apply suggestions from code review

9d487e2

Co-authored-by: Christian Clauss <cclauss@me.com> Signed-off-by: jepietryga <jacobpietryga@gmail.com>

Set noqa to be C901 only

844e5f1

Signed-off-by: jepietryga <jacobpietryga@gmail.com>

jepietryga force-pushed the doneButton branch from 57f18be to 844e5f1 Compare September 1, 2021 14:17

cjh1 approved these changes Sep 1, 2021

View reviewed changes

cclauss approved these changes Sep 1, 2021

View reviewed changes

cjh1 merged commit 88f8395 into OpenChemistry:master Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

'Done' Button Implementation #2177

'Done' Button Implementation #2177

jepietryga commented Jun 3, 2021

cjh1 left a comment

cjh1 Aug 16, 2021

cjh1 Aug 16, 2021

cjh1 Aug 16, 2021

cjh1 Aug 16, 2021

cjh1 Aug 16, 2021

cjh1 commented Aug 18, 2021

jepietryga commented Aug 18, 2021

Hovden commented Aug 25, 2021

cjh1 commented Aug 25, 2021

jepietryga commented Aug 25, 2021 •

edited

Loading

cjh1 commented Aug 25, 2021

cjh1 left a comment

cjh1 Aug 27, 2021

cjh1 Aug 27, 2021

cjh1 Aug 27, 2021

jepietryga commented Aug 27, 2021

jepietryga commented Aug 28, 2021 •

edited

Loading

cjh1 left a comment

'Done' Button Implementation #2177

'Done' Button Implementation #2177

Conversation

jepietryga commented Jun 3, 2021

cjh1 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

cjh1 commented Aug 18, 2021

jepietryga commented Aug 18, 2021

Hovden commented Aug 25, 2021

cjh1 commented Aug 25, 2021

jepietryga commented Aug 25, 2021 • edited Loading

cjh1 commented Aug 25, 2021

cjh1 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

jepietryga commented Aug 27, 2021

jepietryga commented Aug 28, 2021 • edited Loading

cjh1 left a comment

Choose a reason for hiding this comment

jepietryga commented Aug 25, 2021 •

edited

Loading

jepietryga commented Aug 28, 2021 •

edited

Loading