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

More informative error message for error reading InferenceData from NetCDF #1637

Merged
merged 4 commits into from
Mar 30, 2021

Conversation

rpgoldman
Copy link
Contributor

@rpgoldman rpgoldman commented Mar 26, 2021

Description

Previously, when trying to read an InferenceData object using from_netcdf on a filesystem that did not support HDF5 file locking (full disclosure: I have no idea what HDF5 file locking is), one would get a cryptic OSError: -101 which did not provide any assistance in fixing the problem.

I tracked this down, and found a solution on StackOverflow. Because it's difficult to figure out (happens due to a dependency that is well down in the call tree), I tweaked from_netcdf to trap this error, and add an explanation.

Checklist

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)

Remarks

WRT the code style, running pylint locally I get it complaining that the use of e as a variable name is bad, but this is quite normal usage, stylistically, for variables that will be bound to Exception's.

@rpgoldman rpgoldman requested a review from ColCarroll March 26, 2021 17:53
@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #1637 (8aa810d) into main (23e14fb) will decrease coverage by 0.02%.
The diff coverage is 69.23%.

❗ Current head 8aa810d differs from pull request most recent head c7eba46. Consider uploading reports for the commit c7eba46 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1637      +/-   ##
==========================================
- Coverage   90.91%   90.89%   -0.03%     
==========================================
  Files         108      108              
  Lines       11671    11671              
==========================================
- Hits        10611    10608       -3     
- Misses       1060     1063       +3     
Impacted Files Coverage Δ
arviz/data/inference_data.py 83.14% <69.23%> (-0.46%) ⬇️
arviz/plots/posteriorplot.py 91.17% <0.00%> (-0.50%) ⬇️
arviz/rcparams.py 93.47% <0.00%> (-0.49%) ⬇️
arviz/data/io_pystan.py 95.68% <0.00%> (-0.41%) ⬇️
arviz/data/io_cmdstanpy.py 96.12% <0.00%> (-0.38%) ⬇️
arviz/data/io_cmdstan.py 90.90% <0.00%> (-0.05%) ⬇️
arviz/stats/stats.py 96.67% <0.00%> (-0.01%) ⬇️
arviz/data/io_pymc3.py 90.75% <0.00%> (ø)
arviz/plots/distplot.py 90.47% <0.00%> (ø)
arviz/data/io_numpyro.py 94.48% <0.00%> (+0.59%) ⬆️
... and 2 more

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 99764f6...c7eba46. Read the comment docs.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only needs to be added to the changelog

else:
groups[group] = data
return InferenceData(**groups)
except OSError as e: # pylint: disable=invalid-name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use e (instead of err or something a bit longer that pylint considers optimal) as variable to raise errors from other errors, it may be worth it to add it to .pylintrc instead of having to disable the error every time

@rpgoldman
Copy link
Contributor Author

@OriolAbril @ColCarroll This is ready to go, if you are OK with it, please squash and merge.

@OriolAbril OriolAbril merged commit 1b5d24f into arviz-devs:main Mar 30, 2021
utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
…etCDF (arviz-devs#1637)

* More informative error message.

* Blacken and pylint.

* Update Changelog for PR 1637.

Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
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