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

edit groups in idata.extend #1386

Merged
merged 9 commits into from
Sep 22, 2020
Merged

edit groups in idata.extend #1386

merged 9 commits into from
Sep 22, 2020

Conversation

OriolAbril
Copy link
Member

Description

idata.extend does add the groups but does not edit the _groups attributes which makes idata.posterior to work but posterior is not shown when printing nor in the html repr.

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

@canyon289
Copy link
Member

Good thing we got out CI working! Seems like theres an indentation error hidden in there

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1386 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1386      +/-   ##
==========================================
+ Coverage   91.73%   91.74%   +0.01%     
==========================================
  Files         105      105              
  Lines       10909    10915       +6     
==========================================
+ Hits        10007    10014       +7     
+ Misses        902      901       -1     
Impacted Files Coverage Δ
arviz/data/base.py 97.50% <ø> (ø)
arviz/data/inference_data.py 84.12% <100.00%> (+0.28%) ⬆️
arviz/data/io_dict.py 92.91% <100.00%> (+0.17%) ⬆️

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 652302f...69be157. Read the comment docs.

@@ -69,11 +69,15 @@ def __init__(
self.attrs.pop("created_at", None)
self.attrs.pop("arviz_version", None)

@requires("posterior")
def _init_dict(self, attr_name):
dict_or_none = getattr(self, attr_name, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this return empty dict instead of None?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, much better, it directly takes care about the if else below, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

now I realized the if else is what does the actual work, for now all attrs are set, but it's nice to have this here in case we extended io_dict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to update the block below too

Copy link
Member Author

Choose a reason for hiding this comment

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

The block below needs to check for None. This first one catches attributes that have not been set and the one below catches attributes set to None

@ahartikainen ahartikainen merged commit 89a87ef into master Sep 22, 2020
@OriolAbril OriolAbril deleted the idata_fixes branch September 22, 2020 18:52
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.

3 participants