Filter warnings #764

andLaing · 2020-12-23T18:23:31Z

Suppresses expected warnings in MC writer and tests as well as tidying imports.

jacg

Please add closes #762 to the commit message of this commit.

jacg · 2020-12-26T15:30:42Z

I trust that we agree that, wherever possible, rather than suppressing or ignoring the warning, the cause of the warning should be removed.

In some cases (for example, in a test that checks the behaviour of the code in precisely the situation that is being warned about) it's obvious that the warning should simply be ignored. In other cases, this isn't obvious.

Can you give some justifications of the cases where warnings are being ignored rather than their causes being removed? Ideally these would appear in the commit messages of the relevant commit (or a comment at the point of suppression).

jacg · 2020-12-26T15:33:27Z

This one still shows up on Travis:

invisible_cities/reco/tbl_functions.py:92
  /home/travis/build/next-exp/IC/invisible_cities/reco/tbl_functions.py:92: DeprecationWarning: invalid escape sequence \d
    pattern = re.compile('NEXT_v\d[_\d+]+[_\w]+?_(?P<fnumber>\d+)_[_\w]+?_(?P<nevts>\d+)\..*',

I think that preceding the string with r (i.e. r'NEXT_v\d...') should do the trick.

invisible_cities/sierpe/fee_test.py

andLaing · 2020-12-26T18:57:01Z

I trust that we agree that, wherever possible, rather than suppressing or ignoring the warning, the cause of the warning should be removed.

Yes, I think they are justified although in some cases there might be alternatives. The OptimizeWarning for example could maybe be fixed but since there's a random generator involved maybe it would just be fixed for certain machines. I've added to the commit messages and put comments where possible to explain the motivation.

This one still shows up on Travis:

Should be we be worried that it only shows up on Travis? Especially since it's a DeprecationWarning, doesn't that mean that we have some version differences between what Travis uses and what's downloaded by the scripts? Seem unstable.

andLaing · 2020-12-26T19:04:26Z

Now two new warnings hace appeared, there might be something going on behind the scenes

jacg · 2020-12-29T12:19:32Z

Apologies for the silence. I am afraid that I will be unavailable for some time.

mmkekic

Probably should also look into filterwarnings of pytest.ini file. One (import error of collections) is fixed in 06f09f9 so maybe you can just cherry-pick it.

mmkekic · 2021-01-11T13:09:29Z

invisible_cities/cities/components_test.py

 def test_mcsensors_from_file_fast_returns_empty(ICDATADIR):
    file_in = os.path.join(ICDATADIR, "nexus_new_kr83m_fast.newformat.sim.h5")
    sns_gen = mcsensors_from_file([file_in], 'new', -7951)
-    first_evt = next(sns_gen)
+    with warnings.catch_warnings():


Here you do expect No binning info available. so maybe you can use

with pytest.warns(UserWarning, match='No binning info available.'): first_evt = next(sns_gen)

mmkekic · 2021-01-11T13:20:26Z

invisible_cities/io/dst_io.py

@@ -109,7 +109,7 @@ def df_writer(h5out              : tb.file.File ,

    data_types = table.dtype
    if len(arr) == 0:
-        warnings.warn(f'dataframe is empty', UserWarning)
+        warnings.warn('dataframe is empty', UserWarning)


I know that this is not strictly related to this PR but I wonder if we really need this warnings. For example, in pandas to_hdf no warning is raised if dataframe is empty, so maybe we should just keep the same behavior, ie removing the warning completely. Especially since it is almost never guaranteed to have non-empty df in our flows...

Anybody care to share an opinion on this?

Does anyone ever notice or appreciate this warning appearing? Can anyone remember an instance when this warning helped them? If not, it should probably go.

Marija and I discussed it and we decided to remove it in the final version of this PR. That's why it shows as outdated above.

I think that it shows as outdated, because you have rebased, and the commit id has changed. In my latest checkout of the branch, I can still see the warning being raised (strangely enough, with the superfluous 'f' which was removed here!)

Strange. It's changed in the first commit of the current history and I can't find anywhere in the other commits where it could have been accidentally reintroduced.

Apologies. I failed to notice that a Git LFS error prevented me from looking at the right branch. I confirm that it's gone from the current version.

jacg · 2021-01-19T10:41:51Z

@andLaing You wrote 'closes issue #762' in the commit message, but I think that the presence of 'issue' between 'closes' and '#762' interferes with the pattern that GitHub recognizes: https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

andLaing · 2021-01-19T10:44:37Z

Ah, ok, it seemed more natural to specify but I should have checked the standard. My bad, I'll edit the commit message now

jacg · 2021-01-19T10:46:55Z

A fresh installation on a pristine Debian 10 with both Python 3.7 and 3.8 passes all tests with zero warnings.

The latest Travis run produces this single warning:

/home/travis/miniconda/envs/IC-3.7-2020-06-16/lib/python3.7/site-packages/pandas/core/dtypes/missing.py:478
  /home/travis/miniconda/envs/IC-3.7-2020-06-16/lib/python3.7/site-packages/pandas/core/dtypes/missing.py:478: RuntimeWarning: invalid value encountered in equal
    return ((left == right) | (isna(left) & isna(right))).all()

Anyone recognize this?

(The Mac OS run fails, but we'll deal with that in a separate PR.)

jacg · 2021-01-19T10:58:37Z

Some of the commit messages contain justifications/explanations of why warnings are suppressed/ignored. These should probably be added as comments in the code itself.

jacg · 2021-01-19T11:02:03Z

Minor niggle: in 'Change string to r-string in tbl_functions_test' the commit message contains 'Suppress DeprecationWarning', but the warning isn't being suppressed (which would require justification) bit it's being heeded (which requires no further explanation).

jacg · 2021-01-19T11:08:01Z

We should also do a review of all the skipped and xfail tests. Probably in a separate PR, unless it can be done quickly.

jacg · 2021-01-19T11:12:39Z

Latest Travis run shows no warnings.

Add justifications (from commit messages) as comments to source code, and I'm ready to approve.

jacg · 2021-01-19T11:21:17Z

Should be we be worried that it only shows up on Travis? Especially since it's a DeprecationWarning, doesn't that mean that we have some version differences between what Travis uses and what's downloaded by the scripts?

In theory, our manage.sh (or Nix, or whatever we choose to use) should give us identical environments on test, production and development machines. It might be something to do with the Travis test profile asking Hypothesis to produce more cases, and thus dodgy cases (which raise warnings) being found more frequently on Travis. In any case, I agree that it's probably flaky, and that we can probably live with it appearing every now and then.

closes next-exp#762

jacg

All stable warnings have been eliminated, either by addressing the issue they point out, or, where appropriate, ignoring them explicitly. In the latter cases, justifications have been added to both the commit messages and as comments to the code.

This eliminates the deluge of warnings that accompanied each run of the test suite, causing all warnings to be ignored by any humans looking at the test output.

We should go through a similar exercise for skipped and xfailing tests in a separate PR.

andLaing requested a review from jacg December 23, 2020 18:23

jacg reviewed Dec 26, 2020

View reviewed changes

invisible_cities/sierpe/fee_test.py Show resolved Hide resolved

andLaing force-pushed the filter_warnings branch from 45acb26 to 3b686f6 Compare December 26, 2020 18:32

mmkekic reviewed Jan 11, 2021

View reviewed changes

jmbenlloch mentioned this pull request Jan 11, 2021

Travis changes #763

Closed

andLaing added 3 commits January 18, 2021 15:27

Remove unnecessary dataframe empty warning from df_writer

3cffd7e

Suppress expected kdst type warning in safe_copy_nexus_eventmap

f62527d

Mark test_df_writer_raises_warning_empty_dataframe xfail

dfa9590

andLaing force-pushed the filter_warnings branch from a4222e1 to ea40530 Compare January 18, 2021 16:19

andLaing force-pushed the filter_warnings branch from ea40530 to 5b3fdd1 Compare January 19, 2021 10:45

andLaing added 6 commits January 19, 2021 12:59

Suppress expected warnings in tests with empty tables

d12fe69

Change unnecessary f-strings to strings

abcfa63

Suppress expected RuntimeErrors in calib_functions_test

3c8b20f

Remove 0 from test_correct_geometry_properly which raised warning

faf160a

Suppress Optimize warning in test_fit_sum_of_gaussians

5ad3132

Improve test_load_pmaps removing fixture KrMC_pmaps_example

4ed1a8e

andLaing added 7 commits January 19, 2021 13:03

Remove meaningless pytest.mark.feetest from fee_test

882ee95

closes next-exp#762

Tidy unused imports and variables

0beb1e6

Use context manager to manage files in tests where not closed

aec4d55

Heed DeprecationWarning in tbl_functions_test

717841b

Stop ignoring collections.abc import warning

5cf63b1

Heed collections.abc import warning

a01772e

Test expected warning in test_copy_mc_info_split_nexus_events

d1fe416

andLaing force-pushed the filter_warnings branch from 5b3fdd1 to d1fe416 Compare January 19, 2021 12:04

jacg approved these changes Jan 19, 2021

View reviewed changes

carmenromo merged commit 2b42efa into next-exp:master Jan 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter warnings #764

Filter warnings #764

andLaing commented Dec 23, 2020

jacg left a comment

jacg commented Dec 26, 2020

jacg commented Dec 26, 2020

andLaing commented Dec 26, 2020

andLaing commented Dec 26, 2020

jacg commented Dec 29, 2020

mmkekic left a comment

mmkekic Jan 11, 2021

mmkekic Jan 11, 2021

jacg Jan 19, 2021

andLaing Jan 19, 2021

jacg Jan 19, 2021

andLaing Jan 19, 2021

jacg Jan 19, 2021

jacg commented Jan 19, 2021

andLaing commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg left a comment

Filter warnings #764

Filter warnings #764

Conversation

andLaing commented Dec 23, 2020

jacg left a comment

Choose a reason for hiding this comment

jacg commented Dec 26, 2020

jacg commented Dec 26, 2020

andLaing commented Dec 26, 2020

andLaing commented Dec 26, 2020

jacg commented Dec 29, 2020

mmkekic left a comment

Choose a reason for hiding this comment

mmkekic Jan 11, 2021

Choose a reason for hiding this comment

mmkekic Jan 11, 2021

Choose a reason for hiding this comment

jacg Jan 19, 2021

Choose a reason for hiding this comment

andLaing Jan 19, 2021

Choose a reason for hiding this comment

jacg Jan 19, 2021

Choose a reason for hiding this comment

andLaing Jan 19, 2021

Choose a reason for hiding this comment

jacg Jan 19, 2021

Choose a reason for hiding this comment

jacg commented Jan 19, 2021

andLaing commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg commented Jan 19, 2021

jacg left a comment

Choose a reason for hiding this comment