Progress reporting improvements by dzenanz · Pull Request #3014 · InsightSoftwareConsortium/ITK

dzenanz · 2021-12-29T21:29:35Z

Related to #2875.

jhlegarreta

Have not tried, but wondering whether

Now the progress fills up twice faster so it reaches 100% when only 50% of work is done.

is the expected behavior, or an expected limitation (e.g. if it cannot be set to 50% when half of the job has been done, and to be 100% only when the job has been completed), whether the wording is accurate or else whether it is a misunderstanding of mine.

dzenanz · 2021-12-30T15:14:02Z

Before 840bae7 progress would go from 0% to 100% twice, overflowing from 100% to 0% about halfway. After 840bae7, the progress fills up to 100% during first half of the work, and stays there the second half of the work. Now, the progress fills up to 50% during first half of the work, and fills up to 100% during second half of the work.

In all instances, the progress was usually being filled smoothly.

blowekamp

Looks good. I just would like to know why it is needed to set is multiple times in a class hierarchy of constructors

dzenanz · 2021-12-30T15:37:18Z

@jhlegarreta I updated the commit message.

jhlegarreta

Thanks for the patch set and the commit message update, now it makes more sense to me.

After Brad's comment, I'm wondering whether setting/updating the threader update progress report flag should be disabled in child classes/whether that makes sense.

dzenanz · 2021-12-30T17:32:37Z

I have a more comprehensive solution. I am testing it locally.

thewtex · 2021-12-30T18:11:46Z

@dzenanz please start commit messages with capital letters for the ChangeLog.

dzenanz · 2021-12-31T21:30:23Z

There is something wrong with progress updates the second time filter is run:

1570: Trying filter->UpdateLargestPossibleRegion()
1570: -------- Start GradientMagnitudeRecursiveGaussianImageFilter "" Progress Quiet
1570: Filter took 0.0594145 seconds.
1570: -------- End GradientMagnitudeRecursiveGaussianImageFilter ""
1570:
1570: itk::ExceptionObject (000000D8CD2FE3C8)
1570: Location: "void __cdecl itk::SimpleFilterWatcher::EndFilter(void)"
1570: File: C:\Dev\ITK-git\Modules\Core\Common\include\itkSimpleFilterWatcher.h
1570: Line: 283
1570: Description: ITK ERROR: GradientMagnitudeRecursiveGaussianImageFilter(000000D8CD2FED70): Filter does not have progress.
1570:
1570:
1570:   In C:\Dev\ITK-git\Modules\Filtering\ImageGradient\test\itkGradientMagnitudeRecursiveGaussianFilterTest.cxx, line 217
1/1 Test #1570: itkGradientMagnitudeRecursiveGaussianFilterTest ...***Failed    0.47 sec

dzenanz · 2022-01-03T18:00:01Z

something wrong with progress updates the second time filter is run

This is the only problem left. How much of a problem is this? Does someone know how to solve it? I am somewhat tired of this PR. It started as quite a small fix.

dzenanz · 2022-01-03T23:47:15Z

I found out the problem. It will take me some time to clean up the code. I will push the update tomorrow.

blowekamp · 2022-01-04T22:01:41Z

In testing this PR against SimpleITK I got a large number of FFT related test further inailures, and one Progress report test failing. It will need further investigation.

dzenanz · 2022-01-04T22:06:15Z

Thanks for testing. Feel free to push to this this branch if you find some easy fixes.

dzenanz · 2022-01-04T22:45:47Z

PythonLaplacianImageFilterTest deadlocks on my computer too. At least I can reproduce the failure.

dzenanz · 2022-01-04T23:33:16Z

CI Linux Python:

	209 - PythonGradientMagnitudeRecursiveGaussianImageFilterTest (Timeout)
	210 - itkGradientVectorFlowImageFilterPythonTest (Timeout)
	211 - PythonLaplacianImageFilterTest (Timeout)
	215 - PythonGeodesicActiveContourLeftVentricleTest (Timeout)

dzenanz · 2022-01-05T20:33:01Z

I have removed the problematic commit from this PR. I will see if I can iron it out in a follow-up PR. I think this can be merged now.

Filter restarts can occur with streaming (processing the image in pieces) and with internal mini-pipelines which are executed multiple times. One such example is GradientMagnitudeRecursiveGaussianImageFilter.

If the wait_for completed successfully, two updates were issues in quick succession. That no longer happens.

dzenanz · 2022-01-05T22:40:11Z

I removed all far-reaching code from this PR, and left all the non-problematic code here. It probably no longer fixes #2875, so I changed the description accordingly. I plan the real fix for a follow-up PR.

jhlegarreta

👍 Thanks @dzenanz. Given that @blowekamp tested a previous version against SimpleITK, may be he would like to take that again, but I'm for merging it as the one-but-last build passed tests, and hope this will as well.

blowekamp · 2022-01-05T23:03:23Z

Having to reset internal filter's progress when re-running seems like a red flag to me. I'd like to test with just re-running a filter in a standalone test to alleviate my concern. I'm not sure I'd have time tomorrow. There are a lot of changes here.

blowekamp

I tested a few cases that were of concerned to me, and they all behaved correctly 🥳

Also SimpleITK tests seemed to be OK too (although there are other FFT issues).

Nice work!

dzenanz · 2022-01-06T16:15:53Z

Are the FFT issues captured in #3048?

tbirdso · 2022-01-06T16:33:22Z

Also SimpleITK tests seemed to be OK too (although there are other FFT issues).

@blowekamp Could you please add @dzenanz and myself to FFT discussion if there are other open issues?

blowekamp · 2022-01-06T17:00:58Z

When I narrow in down in SimpleITK, I'll create an issue if needed. Here is a build with the failure FYI: https://open.cdash.org/viewTest.php?onlyfailed&buildid=7657537

tbirdso · 2022-01-06T17:07:57Z

At a glance I'm guessing that the SimpleITK tests will need to add factory registrations for VNL FFT classes as the segfaulting tests all seem to require classes that depend on FFT filter instantiations. Whenever you get around to taking a closer look I'd recommend cross-referencing #3002 to see how this is being done in the ITK TestKernel as well as ITK GTests.

dzenanz mentioned this pull request Dec 29, 2021

Some filters report progress both from theader and from themselves #2875

Closed

github-actions Bot added area:Filtering Issues affecting the Filtering module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Dec 29, 2021

dzenanz requested review from blowekamp and jhlegarreta December 30, 2021 14:47

jhlegarreta reviewed Dec 30, 2021

View reviewed changes

blowekamp approved these changes Dec 30, 2021

View reviewed changes

Comment thread Modules/Filtering/LabelMap/include/itkLabelMapToLabelImageFilter.hxx Outdated

Comment thread Modules/Filtering/LabelMap/include/itkLabelShapeKeepNObjectsImageFilter.hxx Outdated

dzenanz force-pushed the progress4filters branch from d44632c to e59597f Compare December 30, 2021 15:36

jhlegarreta approved these changes Dec 30, 2021

View reviewed changes

dzenanz force-pushed the progress4filters branch from e59597f to a7a16a8 Compare December 30, 2021 21:19

github-actions Bot added area:Core Issues affecting the Core module and removed area:Filtering Issues affecting the Filtering module labels Dec 30, 2021

dzenanz force-pushed the progress4filters branch from 24c9274 to d46f46b Compare December 31, 2021 19:50

github-actions Bot added the area:Filtering Issues affecting the Filtering module label Dec 31, 2021

dzenanz force-pushed the progress4filters branch 2 times, most recently from 0cf3b4f to 1cd2afe Compare December 31, 2021 21:28

github-actions Bot added the type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct label Dec 31, 2021

dzenanz changed the title ~~BUG: avoid duplicate progress reporting~~ Progress reporting improvements Dec 31, 2021

dzenanz force-pushed the progress4filters branch from 1cd2afe to 8fe2029 Compare January 3, 2022 23:50

github-actions Bot removed the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Jan 3, 2022

thewtex approved these changes Jan 4, 2022

View reviewed changes

dzenanz force-pushed the progress4filters branch from 8fe2029 to 79243ce Compare January 4, 2022 19:46

blowekamp reviewed Jan 4, 2022

View reviewed changes

Comment thread Modules/Core/Common/include/itkProgressAccumulator.h Outdated

dzenanz force-pushed the progress4filters branch from 79243ce to 55ec9c6 Compare January 4, 2022 20:06

BUG: Filter's progress would sometimes regress

f92e9c3

dzenanz force-pushed the progress4filters branch from 55ec9c6 to da3aa28 Compare January 5, 2022 16:32

dzenanz requested a review from blowekamp January 5, 2022 21:34

dzenanz added 7 commits January 5, 2022 17:22

ENH: Better progress reporting for 3 LabelMap filters

b6dc0ac

STYLE: Simplify GradientMagnitudeRecursiveGaussianFilterTest

f6dd83e

BUG: BinaryGeneratorImageFilter's progress did not reach 1.0

ae5ea53

ENH: Make progress more accurate in case of filter restarts

1a05374

Filter restarts can occur with streaming (processing the image in pieces) and with internal mini-pipelines which are executed multiple times. One such example is GradientMagnitudeRecursiveGaussianImageFilter.

ENH: Improve progress for GradientMagnitudeRecursiveGaussianImageFilter

e7dd77f

BUG: properly serialize output in SimpleFilterWatcher

5fd1e36

ENH: Avoid repeating progress in pool multi threader

1a2f9d5

If the wait_for completed successfully, two updates were issues in quick succession. That no longer happens.

dzenanz force-pushed the progress4filters branch from da3aa28 to 1a2f9d5 Compare January 5, 2022 22:30

jhlegarreta reviewed Jan 5, 2022

View reviewed changes

blowekamp approved these changes Jan 6, 2022

View reviewed changes

dzenanz merged commit ecd025e into InsightSoftwareConsortium:master Jan 6, 2022

dzenanz deleted the progress4filters branch January 6, 2022 16:15

dzenanz mentioned this pull request Jan 6, 2022

Better progress reporting #3052

Merged

This was referenced Jan 17, 2022

Smoother progress reporting #3116

Closed

BUG: fixed simultaneous read/write access of same ivar by different t… #3115

Merged

Uh oh!

Conversation

dzenanz commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Dec 30, 2021

Uh oh!

blowekamp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dzenanz commented Dec 30, 2021

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Dec 30, 2021

Uh oh!

thewtex commented Dec 30, 2021

Uh oh!

dzenanz commented Dec 31, 2021

Uh oh!

dzenanz commented Jan 3, 2022

Uh oh!

dzenanz commented Jan 3, 2022

Uh oh!

Uh oh!

blowekamp commented Jan 4, 2022

Uh oh!

dzenanz commented Jan 4, 2022

Uh oh!

dzenanz commented Jan 4, 2022

Uh oh!

dzenanz commented Jan 4, 2022

Uh oh!

dzenanz commented Jan 5, 2022

Uh oh!

dzenanz commented Jan 5, 2022

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

blowekamp commented Jan 5, 2022

Uh oh!

blowekamp left a comment

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Jan 6, 2022

Uh oh!

tbirdso commented Jan 6, 2022

Uh oh!

blowekamp commented Jan 6, 2022

Uh oh!

tbirdso commented Jan 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dzenanz commented Dec 29, 2021 •

edited

Loading