Skip to content

[WIP] Silence std::cerr calls inside of python #377

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

Merged
merged 1 commit into from
May 13, 2022

Conversation

stanleyjs
Copy link
Contributor

Types of changes

Adds flags to disable std::cerr reporting in the cpp backend. Adds parameter ot.backend.use_cerr as parameter and ot.backend.get_cerr() getter to enable/disable cerr reporting inside of python. The parameter defaults to False.

Motivation and context / Related issue

We are using POT inside of jupyter notebook and running into a problem that hits its iter limits. The warnings flood our console. Because the backend code is calling std::cerr directly, rather than passing the error string up to the python stderr stream, traditional context management based redirection (or simply killing sys.stderr inside of python) does not work.

How has this been tested (if it applies)

Have tested the basic use case and enable/disabling the cerr. Works in ipython on 3.9.7.

PR checklist

  • [x ] I have read the CONTRIBUTING document.
  • [x ] The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • [ x] I have added the PR and Issue fix to the RELEASES.md file.

@rflamary
Copy link
Collaborator

Hello @stanleyjs and thank you for your PR. I agree that using std::cerr in the cpp code was a bad move and I've been meaning to change that for a while so your PR is very interesting.

Still I would have done it in a different way. I think we should remove all those printing in the cpp instead of adding a new parameter and use proper python warning when using the result_code in the cython function emd_c. This mean that you will be able to desactivate the warnings but they will still be printed by default (and this time properly). It would be nice to implement this and a test that checks that the warning is printed for a very small number of max iteration. Are you OK with my proposition?

@rflamary rflamary changed the title Silence std::cerr calls inside of python [WIP] Silence std::cerr calls inside of python May 13, 2022
@stanleyjs
Copy link
Contributor Author

@rflamary you mean to just strip out all the std::cerr calls?
I agree that that is cleaner, I just didn't want to remove them because it wasn't clear to me if you wanted the c++ to be useful outside of the python context. In that case, perhaps the c++ error reporting would be useful.

I'll strip it all out and rebase.

@rflamary
Copy link
Collaborator

yep strip out all you like : the cpp is only for wrapping with cython here

bonus here is we don't add an undocumented parameter to the toolbox

typo in releases
@stanleyjs
Copy link
Contributor Author

stanleyjs commented May 13, 2022

@rflamary the newest push has this implemented as just bare removal of cerr.

It appears that the test you requested was already implemented on line 380 : test_warnings() of tests/test_ot.py

@rflamary
Copy link
Collaborator

that's right I forgot about check_result . Thank you

@rflamary rflamary merged commit b001d7a into PythonOT:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants