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

Adapt summary function to circular variables #1313

Merged
merged 8 commits into from
Jul 24, 2020
Merged

Adapt summary function to circular variables #1313

merged 8 commits into from
Jul 24, 2020

Conversation

agustinaarroyuelo
Copy link
Contributor

@agustinaarroyuelo agustinaarroyuelo commented Jul 22, 2020

Description

In this PR I changed the az.summary function to display statistics for circular variables more neatly. It is related to issue #339.

torsionals = az.load_arviz_data('glycan_torsion_angles')
az.summary(torsionals, circ_var_names=['tors'])

outputs:

sc_summary

Checklist

  • Follows official PR format
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

LGTM.

Can show an example (screenshot to this PR is fine)

(I just wanted to see does it create nan for other stats or what)

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.

Can show an example (screenshot to this PR is fine)

Plus now that you have added the glycan example you can add examples to the docstring of using the new argument. If you plan to modify the glycan example maybe it's better to wait for the new example though, just commenting to make sure we document and properly advertise the cool new features.

arviz/stats/stats.py Show resolved Hide resolved
@agustinaarroyuelo
Copy link
Contributor Author

Thanks for your comments @OriolAbril and @ahartikainen. I updated the PR description. As you can see, circular statistics are placed only where the circular variables are, so no need for extra columns.

Maybe waiting for the new example to update the doctrings will be better but I will check with @aloctavodia

@ahartikainen
Copy link
Contributor

That looks great.

This PR doesn't need to address this, but should we add a column for is_circular and use True for variables that are?
Normal summary doesn't need this, but maybe when we use circular variables it might be a good idea.

Also (I forgot already this) are those in degrees or in radians?

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #1313 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1313   +/-   ##
=======================================
  Coverage   93.12%   93.12%           
=======================================
  Files         101      101           
  Lines       10034    10038    +4     
=======================================
+ Hits         9344     9348    +4     
  Misses        690      690           
Impacted Files Coverage Δ
arviz/stats/stats.py 96.27% <100.00%> (+0.02%) ⬆️

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 7d164b4...c8f7ebd. Read the comment docs.


if kind != "diagnostics":
for metric, circ_stat in zip(
metrics[:5], (circ_mean, circ_sd, circ_hdi_lower, circ_hdi_higher, circ_mcse)
Copy link
Member

Choose a reason for hiding this comment

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

Whats 5 here? Would add comment for the magic number

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree about the comment.
The 5 is related to the "stats" columns, the diagnostics are computed the same for linear and circular variables. Actually, we have to think if the diagnostics also need to be changed for circular variables (not part of this PR).

arviz/stats/stats.py Outdated Show resolved Hide resolved
Co-authored-by: Oriol Abril-Pla <[email protected]>
@aloctavodia
Copy link
Contributor

Not sure about having a "true column" for circular variables. I think a better approach is to have that information included in the InferenceData object.

@agustinaarroyuelo
Copy link
Contributor Author

That looks great.

This PR doesn't need to address this, but should we add a column for is_circular and use True for variables that are?
Normal summary doesn't need this, but maybe when we use circular variables it might be a good idea.

Also (I forgot already this) are those in degrees or in radians?

This angles are in radians

@agustinaarroyuelo agustinaarroyuelo changed the title [WIP] Adapt summary function to circular variables Adapt summary function to circular variables Jul 23, 2020
@aloctavodia aloctavodia merged commit b6339eb into arviz-devs:master Jul 24, 2020
@agustinaarroyuelo agustinaarroyuelo deleted the summary branch July 24, 2020 13:21
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.

5 participants