Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Executable progress indication appears suppressed on Windows #756

Closed
N-Dekker opened this issue Nov 16, 2022 · 8 comments · Fixed by #832
Closed

Executable progress indication appears suppressed on Windows #756

N-Dekker opened this issue Nov 16, 2022 · 8 comments · Fixed by #832

Comments

@N-Dekker
Copy link
Member

N-Dekker commented Nov 16, 2022 •
edited
Loading

The following logic does not seem to work on Windows:

elastix/Core/elxProgressCommand.cxx

Lines 39 to 46 in cf02734

/** Check if the output of the stream is a console. */
this->m_StreamOutputIsConsole = false;
std::string streamOutput = "cout";
int currentPos = xl::xout["coutonly"].GetCOutputs().find(streamOutput)->second->tellp();
if (currentPos == -1)
{
this->m_StreamOutputIsConsole = true;
}

See https://godbolt.org/z/vTEr9nfs3


#include <iostream>

int main()
{
    int currentPos = std::cout.tellp(); 
    std::cout << "currentPos = " << currentPos << '\n';
    return currentPos;
}

Output:

Program returned: 0
currentPos = 0

Even though Stack Overflow suggests that it should be -1: why does cout.tellp always return -1?, and Discrimination between file and console streams

The consequence is that the progress indicator won't appear on Windows.
@N-Dekker
Copy link
Member Author

@mstaring Do you possibly remember, is this a known problem? I see you introduces this tellp() logic (Check if the output of the stream is a console) with commit 5a1b41e

@N-Dekker
Copy link
Member Author

N-Dekker commented Nov 17, 2022 •
edited
Loading

It was suggested (by Andrew Marlow at the accu-general mailing list, as well as by other replies at https://stackoverflow.com/questions/18081392/discrimination-between-file-and-console-streams) to use isatty instead of tellp:

@N-Dekker
Copy link
Member Author

@mstaring @stefanklein Looking at

elastix/Core/elxProgressCommand.cxx

Lines 41 to 42 in cf02734

std::string streamOutput = "cout";
int currentPos = xl::xout["coutonly"].GetCOutputs().find(streamOutput)->second->tellp();

Doesn't xl::xout["coutonly"].GetCOutputs().find("cout")->second always just return a pointer to std::cout, by definition? If so, why not just use std::cout directly?

"coutonly" is initialized in elastix::xoutSetup, at:

elastix/Core/Kernel/elxElastixMain.cxx

Lines 97 to 113 in cf02734

returndummy |= g_data.CoutOnlyXout.AddOutput("cout", &std::cout);
/** Copy the outputs to the warning-, error- and standard-xouts. */
g_data.WarningXout.SetOutputs(xl::xout.GetCOutputs());
g_data.ErrorXout.SetOutputs(xl::xout.GetCOutputs());
g_data.StandardXout.SetOutputs(xl::xout.GetCOutputs());
g_data.WarningXout.SetOutputs(xl::xout.GetXOutputs());
g_data.ErrorXout.SetOutputs(xl::xout.GetXOutputs());
g_data.StandardXout.SetOutputs(xl::xout.GetXOutputs());
/** Link the warning-, error- and standard-xouts to xout. */
returndummy |= xl::xout.AddTargetCell("warning", &g_data.WarningXout);
returndummy |= xl::xout.AddTargetCell("error", &g_data.ErrorXout);
returndummy |= xl::xout.AddTargetCell("standard", &g_data.StandardXout);
returndummy |= xl::xout.AddTargetCell("logonly", &g_data.LogOnlyXout);
returndummy |= xl::xout.AddTargetCell("coutonly", &g_data.CoutOnlyXout);

@stefanklein
Copy link
Member

stefanklein commented Nov 17, 2022 via email

@N-Dekker
Copy link
Member Author

Perhaps it was written in a way to make it more generic, and be prepared for any future extensions that never materialized.

Thanks @stefanklein Would you then agree that it's easier now to use std::cout instead of xl::xout["coutonly"]?

elastix/Core/elxProgressCommand.cxx

Line 186 in cf02734

xl::xout["coutonly"] << "\r" << this->m_StartString << progressInt << this->m_EndString;

xl::xout["coutonly"] means std::cout only, right?

N-Dekker added a commit that referenced this issue Nov 17, 2022
@N-Dekker
BUG: Remove elx::ProgressCommand::m_StreamOutputIsConsole
461631c 
Addresses issue #756 "Executable progress indication appears suppressed on Windows"
@stefanklein
Copy link
Member

stefanklein commented Nov 17, 2022 via email

@N-Dekker
Copy link
Member Author

Could it be the case that there is some setting in the parameter file that turns off any output to screen? So that in this case anything that is written xl::xout["coutonly"] is ignored? Perhaps that would be a reason to keep this wrap around std::cout?

@stefanklein Possible, I'd have to have another look, thanks.

In the mean time, I made an experimental branch that unconditionally writes the progress indication to std::cout for elastix executables:

main...Remove-ProgressCommand-m_StreamOutputIsConsole

Unfortunately one of the tests fail on Windows now:

97 - elastix_run_3DCT_lung.SSD.bspline.ASGD.002_COMPARE_OVERLAP (Timeout)

I guess the ProgressCommand has too much slowed down the execution. It does produce progress output, even to the CI log file:

2022-11-17T23:06:48.0420807Z   Progress: 0%
2022-11-17T23:06:48.0421430Z   Progress: 11%
2022-11-17T23:06:48.0422470Z   Progress: 12%
2022-11-17T23:06:48.0423188Z   Progress: 14%
2022-11-17T23:06:48.0423811Z   Progress: 16%
2022-11-17T23:06:48.0424441Z   Progress: 18%
2022-11-17T23:06:48.0425029Z   Progress: 29%
2022-11-17T23:06:48.0425612Z   Progress: 31%
2022-11-17T23:06:48.0426196Z   Progress: 33%
2022-11-17T23:06:48.0426769Z   Progress: 35%
2022-11-17T23:06:48.0427347Z   Progress: 36%
2022-11-17T23:06:48.0427924Z   Progress: 38%

https://dev.azure.com/kaspermarstal/d9c40921-2d83-43b7-b98b-be691988c03a/_apis/build/builds/3627/logs/48

Maybe we should just add a command-line option for elastix, /progress off or /progress on. The progress indicator is somewhat special, compared to other logging info, as progress indicator should not produce new-lines. It should just rewrite the same line over and over.

@N-Dekker
Copy link
Member Author

@stefanklein At the sprint of last Monday, Marius (@mstaring) decided not to directly fix this issue right now. Instead, it may be addressed later, with an upgrade or redesign of the logging system of elastix/transformix.

N-Dekker added a commit that referenced this issue Feb 2, 2023
@N-Dekker
STYLE: Replace xout["coutonly"] by std::cout, in ProgressCommand
3cdc677 
Note that this commit does not yet address issue #756 "Executable progress indication appears suppressed on Windows"
N-Dekker added a commit that referenced this issue Feb 3, 2023
@N-Dekker
STYLE: Replace xout["coutonly"] by std::cout, in ProgressCommand
9d154e5 
Note that this commit does not yet address issue #756 "Executable progress indication appears suppressed on Windows"
N-Dekker added a commit that referenced this issue Feb 6, 2023
@N-Dekker
STYLE: Replace xl::xout["coutonly"] with std::cout, in ProgressCommand
27de223 
Note that this commit does not yet address issue #756 "Executable progress indication appears suppressed on Windows"
@N-Dekker N-Dekker mentioned this issue Feb 6, 2023
Merged
N-Dekker added a commit that referenced this issue Feb 6, 2023
@N-Dekker
STYLE: Replace xl::xout["coutonly"] with std::cout, in ProgressCommand
a877619 
Note that this commit does not yet address issue #756 "Executable progress indication appears suppressed on Windows"
N-Dekker added a commit that referenced this issue Mar 3, 2023
@N-Dekker
BUG: Remove StreamOutputIsConsole from ProgressCommand
fa61170 
The check if `std::cout.tellp() == -1` does not work (anymore...)

Addresses issue #756 "Executable progress indication appears suppressed on Windows".
@N-Dekker N-Dekker mentioned this issue Mar 4, 2023
Closed
N-Dekker added a commit that referenced this issue Mar 11, 2023
@N-Dekker
BUG: Remove StreamOutputIsConsole from ProgressCommand
db0a0ce 
The check if `std::cout.tellp() == -1` does not work (anymore...)

Addresses issue #756 "Executable progress indication appears suppressed on Windows".
@N-Dekker N-Dekker mentioned this issue Mar 11, 2023
Merged
@N-Dekker N-Dekker linked a pull request Mar 13, 2023 that will close this issue
Support "ShowProgressPercentage" parameter (false by default) #832
Merged
N-Dekker added a commit that referenced this issue Mar 13, 2023
@N-Dekker
BUG: Remove StreamOutputIsConsole from ProgressCommand
67001a4 
The check if `std::cout.tellp() == -1` does not work (anymore...)

Addresses issue #756 "Executable progress indication appears suppressed on Windows".
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support "ShowProgressPercentage" parameter (false by default) SuperElastix/elastix
2 participants
@stefanklein @N-Dekker