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

Resolve RcParams MRO confusion and delete unnecessary methods. #1494

Merged
merged 5 commits into from
Jan 13, 2021

Conversation

obi1kenobi
Copy link
Contributor

Description

Working to resolve the following issue raised by mypy:

arviz/rcparams.py:286: error: Cannot determine consistent method resolution order (MRO) for "RcParams"  [misc]

It seems that RcParams inherits from both dict and MutableMapping, which is confusing mypy because dict is also a MutableMapping. Additionally, it seems that this confusing MRO situation has led to the need to redefine several of the MutableMapping methods explicitly, to avoid MRO-related dispatch issues.

I'm not sure why RcParams needs MutableMapping in addition to having dict as a superclass, so I'm leaving this PR as a draft for now. None of the documentation for the class seems to explain this quirk, so I'm hoping that either test cases surface some leads, or one of the maintainers recalls what the reason might be.

If the tests don't show any issues, and none of the maintainers see a reason to have the multiple inheritance setup and its associated MRO issues, then it may be worth removing MutableMapping as this PR does, and clearing up the mypy error and the two associated (and currently suppressed) pylint errors.

Checklist

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

@OriolAbril
Copy link
Member

The double inheritance is copied from matplotlib whose rcparams class is the base for ArviZ's one. I probably assumed matplotlib devs had a reason for it and kept going. Having said that, even if there was a reason for matplotlib to do this, ArviZ's rcparams are a simplified version of matplotlib ones so if everything works and all test pass let's simplify this.

@codecov
Copy link

codecov bot commented Jan 11, 2021

Codecov Report

Merging #1494 (3762779) into master (910e781) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1494   +/-   ##
=======================================
  Coverage   91.97%   91.97%           
=======================================
  Files         105      105           
  Lines       11239    11241    +2     
=======================================
+ Hits        10337    10339    +2     
  Misses        902      902           
Impacted Files Coverage Δ
arviz/rcparams.py 93.47% <100.00%> (+0.34%) ⬆️
arviz/stats/stats.py 96.27% <0.00%> (-0.14%) ⬇️

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 910e781...3762779. Read the comment docs.

@obi1kenobi
Copy link
Contributor Author

I believe the current implementation should work. I removed the dict inheritance and simply used an explicit underlying dict to store the data — essentially, using composition instead of inheritance. I then implemented the methods MutableMapping requires, and rely on MutableMapping to implement the rest of the interface on the basis of those methods.

@obi1kenobi obi1kenobi marked this pull request as ready for review January 11, 2021 05:24
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.

Thanks! This looks great

@OriolAbril OriolAbril merged commit 3878e17 into arviz-devs:master Jan 13, 2021
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