Modify compare() docstring, error-check, pass var_name. #1616

rpgoldman · 2021-03-15T16:02:22Z

Description

Check to see if the log_likelihood groups have more than one data
variable, and if so, require the var_name parameter. This avoids
having a less understandable error message crop up later.

Introduce var_name parameter to compare(), and pass it through to subsidiary IC function. @OriolAbril reports it was simply an oversight that this wasn't done before.

Addresses issue #1614

Checklist

Follows official PR format
Includes new or updated tests to cover the new feature
Code style correct (follows pylint and black guidelines)
Changes are listed in changelog

arviz/stats/stats.py

rpgoldman · 2021-03-20T19:29:03Z

Looking at the test failures, it seems that sometimes ArviZ computes the log_likelihood group for input InferenceData objects, instead of just extracting it. That was not what I expected. It suggests that this should be redone as I suggested above: catch the errors from the ic_func and annotate them, instead of doing the error-check in compare()

OriolAbril · 2021-03-20T19:35:58Z

Looking at the test failures, it seems that sometimes ArviZ computes the log_likelihood group for input InferenceData objects, instead of just extracting it. That was not what I expected. It suggests that this should be redone as I suggested above: catch the errors from the ic_func and annotate them, instead of doing the error-check in compare()

Maybe the test helpers have not been updated and still have log_likelihood as a variable in sample_stats. The get_log_likelihood prints a warning if this happens but it still works for backward compatibility reasons.

rpgoldman · 2021-03-20T19:40:43Z

I tried the error-catching and re-raising approach and it worked locally for me, so I am pushing it for inspection and testing.

I still need to add some new tests to verify that the errors are signaled as I expect.

arviz/stats/stats.py

Check to see if the log_likelihood groups have more than one data variable, and if so, require the var_name parameter. This avoids having a less understandable error message crop up later. Introduce `var_name` parameter, and pass it through to the IC function invoked by compare.

Previously, if we got a TypeError when trying to find a log_likelihood in from_pymc3() that TypeError would be squashed completely. Now we will echo it to the log before handling it.

@OriolAbril

Took @OriolAbril correction.

Catch errors from IC functions invoked inside compare and annotate them with information about the source `InferenceData` object.

rpgoldman · 2021-03-26T20:13:59Z

@OriolAbril I think this now does things the way you wanted: I grab up the errors and signal a new error from the old one, with additional information identifying which model caused the problem. I'll look at your cross-reference now.

Many examples show this as a good variable name for exceptions, particularly in "except <ExceptionClass> as e:"

Error now caught sooner.

codecov · 2021-03-27T19:28:02Z

Codecov Report

Merging #1616 (6d27fd5) into main (23e14fb) will decrease coverage by 0.02%.
The diff coverage is 88.09%.

❗ Current head 6d27fd5 differs from pull request most recent head 1a31db2. Consider uploading reports for the commit 1a31db2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1616      +/-   ##
==========================================
- Coverage   90.91%   90.89%   -0.03%     
==========================================
  Files         108      108              
  Lines       11671    11700      +29     
==========================================
+ Hits        10611    10635      +24     
- Misses       1060     1065       +5

Impacted Files	Coverage Δ
arviz/data/io_pymc3.py	`90.20% <75.00%> (-0.56%)`	⬇️
arviz/stats/stats.py	`96.28% <88.46%> (-0.40%)`	⬇️
arviz/rcparams.py	`94.16% <100.00%> (+0.20%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23e14fb...1a31db2. Read the comment docs.

.pylintrc

arviz/data/io_pymc3.py

arviz/rcparams.py

Make sure we check for multiple observed variables in compare() and that support for "var_name" works.

Previously, we checked for `sample_stats` in `get_log_likelihood()` *before* reading `log_likelihood`. Add a check that `log_likelihood` must be missing before we check `sample_stats`.

OriolAbril

I added a couple minor comments, looks good

arviz/stats/stats.py

arviz/tests/base_tests/test_stats.py

* Limit recomputation in tests by scoping a fixture. * Test for expected IC in `compare` test. * Refine type assertion.

rpgoldman · 2021-03-29T21:58:21Z

@OriolAbril If my changes meet with your approval, please squash and merge.

@OriolAbril

) * Modify compare() docstring, and var_name param and error-check. Check to see if the log_likelihood groups have more than one data variable, and if so, require the var_name parameter. This avoids having a less understandable error message crop up later. Introduce `var_name` parameter, and pass it through to the IC function invoked by compare. * Fix error in argument test. * More explicit error message. Previously, if we got a TypeError when trying to find a log_likelihood in from_pymc3() that TypeError would be squashed completely. Now we will echo it to the log before handling it. * Fix type signature of compare(). Took @OriolAbril correction. * Annotated IC function errors from compare(). Catch errors from IC functions invoked inside compare and annotate them with information about the source `InferenceData` object. * Remove incorrect docstring. * pylint * mypy fixes. * Make "e" acceptable variable name. Many examples show this as a good variable name for exceptions, particularly in "except <ExceptionClass> as e:" * Changelog update. * Backward-compatibility fix. * Fix test. Error now caught sooner. * Fix mypy issues. * Python 3.5 and 3.6 compatibility. * Whitespace issue caught by Oriol. * Test for error-trapping. Make sure we check for multiple observed variables in compare() and that support for "var_name" works. * Don't let sample_stats shadow log_likelihood. Previously, we checked for `sample_stats` in `get_log_likelihood()` *before* reading `log_likelihood`. Add a check that `log_likelihood` must be missing before we check `sample_stats`. * Improvements suggested by OriolAbril. * Limit recomputation in tests by scoping a fixture. * Test for expected IC in `compare` test. * Refine type assertion.

rpgoldman requested a review from OriolAbril March 15, 2021 16:02

rpgoldman added the Enhancement Improvements to ArviZ label Mar 15, 2021

rpgoldman linked an issue Mar 15, 2021 that may be closed by this pull request

Error comparing PyMC3 models with multiple observed variables #1614

Closed

OriolAbril reviewed Mar 15, 2021

View reviewed changes

arviz/stats/stats.py Outdated Show resolved Hide resolved

arviz/stats/stats.py Outdated Show resolved Hide resolved

OriolAbril requested changes Mar 20, 2021

View reviewed changes

arviz/stats/stats.py Outdated Show resolved Hide resolved

OriolAbril mentioned this pull request Mar 26, 2021

WAIC/LOO for models with multiple observed variables #987

Closed

rpgoldman added 6 commits March 26, 2021 14:53

Fix error in argument test.

be8c21d

More explicit error message.

ce480fd

Previously, if we got a TypeError when trying to find a log_likelihood in from_pymc3() that TypeError would be squashed completely. Now we will echo it to the log before handling it.

Fix type signature of compare().

63c4d06

Took @OriolAbril correction.

Annotated IC function errors from compare().

37c2e81

Catch errors from IC functions invoked inside compare and annotate them with information about the source `InferenceData` object.

Remove incorrect docstring.

b50b6de

rpgoldman force-pushed the iss1614 branch from 9983d01 to b50b6de Compare March 26, 2021 19:54

pylint

e850385

rpgoldman force-pushed the iss1614 branch from 9325310 to e850385 Compare March 26, 2021 20:21

OriolAbril approved these changes Mar 26, 2021

View reviewed changes

rpgoldman and others added 8 commits March 26, 2021 16:51

mypy fixes.

c367270

Merge branch 'main' into iss1614

50ee13c

Make "e" acceptable variable name.

cf2e8b5

Many examples show this as a good variable name for exceptions, particularly in "except <ExceptionClass> as e:"

Changelog update.

7b0ad6e

Backward-compatibility fix.

31be080

Fix test.

6edc71e

Error now caught sooner.

Fix mypy issues.

7eb162c

Python 3.5 and 3.6 compatibility.

6d27fd5

OriolAbril reviewed Mar 27, 2021

View reviewed changes

.pylintrc Outdated Show resolved Hide resolved

arviz/data/io_pymc3.py Show resolved Hide resolved

arviz/rcparams.py Show resolved Hide resolved

Whitespace issue caught by Oriol.

9e61c58

rpgoldman mentioned this pull request Mar 28, 2021

Fix issues with python 3.6/3.7 support and typing #1638

Closed

rpgoldman added 2 commits March 29, 2021 12:02

Test for error-trapping.

1a31db2

Make sure we check for multiple observed variables in compare() and that support for "var_name" works.

Don't let sample_stats shadow log_likelihood.

a6d14ef

Previously, we checked for `sample_stats` in `get_log_likelihood()` *before* reading `log_likelihood`. Add a check that `log_likelihood` must be missing before we check `sample_stats`.

rpgoldman marked this pull request as ready for review March 29, 2021 17:26

OriolAbril reviewed Mar 29, 2021

View reviewed changes

arviz/stats/stats.py Outdated Show resolved Hide resolved

arviz/tests/base_tests/test_stats.py Outdated Show resolved Hide resolved

arviz/tests/base_tests/test_stats.py Outdated Show resolved Hide resolved

Improvements suggested by OriolAbril.

29a03ea

* Limit recomputation in tests by scoping a fixture. * Test for expected IC in `compare` test. * Refine type assertion.

rpgoldman changed the title ~~WIP: Modify compare() docstring, error-check, pass var_name.~~ Modify compare() docstring, error-check, pass var_name. Mar 30, 2021

OriolAbril merged commit d96880c into arviz-devs:main Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify compare() docstring, error-check, pass var_name. #1616

Modify compare() docstring, error-check, pass var_name. #1616

rpgoldman commented Mar 15, 2021 •

edited

Loading

rpgoldman commented Mar 20, 2021

OriolAbril commented Mar 20, 2021

rpgoldman commented Mar 20, 2021

rpgoldman commented Mar 26, 2021

codecov bot commented Mar 27, 2021 •

edited

Loading

OriolAbril left a comment

rpgoldman commented Mar 29, 2021

Modify compare() docstring, error-check, pass var_name. #1616

Modify compare() docstring, error-check, pass var_name. #1616

Conversation

rpgoldman commented Mar 15, 2021 • edited Loading

Description

Checklist

rpgoldman commented Mar 20, 2021

OriolAbril commented Mar 20, 2021

rpgoldman commented Mar 20, 2021

rpgoldman commented Mar 26, 2021

codecov bot commented Mar 27, 2021 • edited Loading

Codecov Report

OriolAbril left a comment

Choose a reason for hiding this comment

rpgoldman commented Mar 29, 2021

rpgoldman commented Mar 15, 2021 •

edited

Loading

codecov bot commented Mar 27, 2021 •

edited

Loading