Skip to content
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

ignore swiglal warnings #4801

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ignore swiglal warnings #4801

wants to merge 1 commit into from

Conversation

ahnitz
Copy link
Member

@ahnitz ahnitz commented Jun 26, 2024

lal adds a warning now whenever it is imported. This is somewhat annoying especially in a notebook. I think we should just silence these.

@spxiwh
Copy link
Contributor

spxiwh commented Jun 26, 2024

I'm not in favour of this.

The warning was added once lal solved an issue we (and others) had long complained about, specifically that if a C-level error occurs, we only get some generic warning, with no real idea of what wen wrong, at the python level. This is now enabled by default in notebooks, but doing so does come with a performance penalty. I think PyCBC users might have cases where it is best to use with lal.no_swig_redirect_standard_output_error(): and cases where not, but this is hidden if we silence the warning intended to advertise this.

I've no issue with silencing this warning in tutorial notebooks (with a comment), but am not sure it should be put here (silently making this decision for any user that imports PyCBC).

@ahnitz
Copy link
Member Author

ahnitz commented Jul 2, 2024

@spxiwh I don't understand the choice of defaults here. Why is it different between a notebook and a script? A notebook is exactly where I wouldn't care as much about performance, so why the scary message that pops up?

I don't think the right solution anytime there are potential user environment choices to have a forced warning. If all packages did this, it would be untenable.

Wouldn't the better thing to be do have it off by default, but if an error occurs they get the complement of this message on how to get a more detailed error report?

@spxiwh
Copy link
Contributor

spxiwh commented Jul 4, 2024

"Why is it different between a notebook and a script?" -> A script (or python, or ipython window) will not swallow error messages raised at the C level. A jupyter notebook will. So if (for example) I try to generate SEOBNR and don't have the frequency high enough to include ringdown, the python error is just "RuntimeError: Internal function call failed: Input domain error", whereas the C error will tell you what the problem is. In the script you see it all, in Jupyter you do not. ... You will not see this warning from within a script.

Your suggestion at the end makes sense to me, but I don't know enough about SWIGLAL to comment on why it wasn't done that way, or if it is possible.

@ahnitz
Copy link
Member Author

ahnitz commented Jul 4, 2024

@spxiwh My guess is that it just wasn't suggested, probably not anything deep into any choice. I think the fact that swiglal was already able to raise a RuntimError with a generic "failed" message means that it should be possible. In fact, we could probably implement it ourselves.

For example, we could

(1) disable the error by default (but have some way to re-eanble?)
(2) put the lal* libraries uses in a try/except whereby the runtimeerror is caught and a message on how to enable a more detailed message is raised (if not already enabled).

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants