search

gwastro / pycbc

Core package to analyze gravitational-wave data, find signals, and study their parameters. This package was used in the first direct detection of gravitational waves (GW150914), and is used in the ongoing analysis of LIGO/Virgo data.
http://pycbc.org
GNU General Public License v3.0
307 stars 344 forks source link

ignore swiglal warnings #4801

Open ahnitz opened 2 weeks ago

ahnitz commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

@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 commented 1 week ago

"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 commented 1 week ago

@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?