extend 'right' join bug fix

[mjhajharia]

Bug:

idata.extend(another_idata, join='right') returns two copies of every group - i.e. posterior, prior

Bug cause:
append should only happen if group not in self

Possible fix - (thanks @OriolAbril )

Note: extend(join='left') works just fine.

Feel free to suggest better fixes or tests or anything needed

[OriolAbril]

Regarding tests, I think this one https://github.com/arviz-devs/arviz/blob/main/arviz/tests/base_tests/test_data.py#L782 should be modified to check that not only the attributes are right but the list stored in _groups is too.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[mjhajharia]

Regarding tests, I think this one https://github.com/arviz-devs/arviz/blob/main/arviz/tests/base_tests/test_data.py#L782 should be modified to check that not only the attributes are right but the list stored in _groups is too.

i'm sorry, i'm not sure i understand

[OriolAbril]

the code file probably changed and the line is no longer right. This test https://github.com/arviz-devs/arviz/blob/main/arviz/tests/base_tests/test_data.py#L791-L808 checks that the extended inferencedata has the right attributes with hasattr and that worked already. But having the right attributes is not enough for extend to have worked properly. We have to check that the _groups list is also correctly updated and matches the actual attributes in the inferencedata. The bug was due to _groups being wrong, so while the result inferencedata was "right" and had all the extended attributes, _groups (which is what is used for printing) was not, so it looked like there were wrong and/or repeated attributes.

[ahartikainen]

I added some extra logic to keep the supported order for the groups (if groups are still in that order)

[codecov]

Codecov Report

Merging #1718 (06cfa92) into main (d90c79d) will increase coverage by 0.04%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
+ Coverage   90.86%   90.90%   +0.04%     
==========================================
  Files         108      113       +5     
  Lines       11821    12239     +418     
==========================================
+ Hits        10741    11126     +385     
- Misses       1080     1113      +33     

Impacted Files	Coverage Δ
arviz/data/inference_data.py	83.76% <96.15%> (+0.19%)	⬆️
arviz/data/io_pymc3.py	73.33% <0.00%> (-16.87%)	⬇️
arviz/plots/ppcplot.py	89.33% <0.00%> (-4.52%)	⬇️
arviz/sel_utils.py	83.82% <0.00%> (-4.42%)	⬇️
arviz/plots/backends/matplotlib/distplot.py	92.30% <0.00%> (-3.15%)	⬇️
arviz/plots/backends/bokeh/distplot.py	84.37% <0.00%> (-1.99%)	⬇️
arviz/data/io_cmdstanpy.py	93.75% <0.00%> (-1.69%)	⬇️
arviz/utils.py	88.23% <0.00%> (-0.91%)	⬇️
arviz/plots/backends/bokeh/kdeplot.py	95.89% <0.00%> (-0.85%)	⬇️
arviz/rcparams.py	93.33% <0.00%> (-0.84%)	⬇️
... and 65 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 d90c79d...06cfa92. Read the comment docs.

[OriolAbril]

looks fixed but the test is still missing

I added some extra logic to keep the supported order for the groups (if groups are still in that order)

don't we take care of that when printing?

[ahartikainen]

Hmm not sure.

I can add tests to my fix pr

[ahartikainen]

If we do handle the order while printing, then I will remove that extra logic

[OriolAbril]

If we do handle the order while printing, then I will remove that extra logic

I remembered wrong, it's in init: https://github.com/arviz-devs/arviz/blob/main/arviz/data/inference_data.py#L142

[ahartikainen]

Ok, I will leave in place. Thanks for checking it out