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

integrate index_origin rcParam #984

Closed
OriolAbril opened this issue Jan 5, 2020 · 8 comments · Fixed by #1201
Closed

integrate index_origin rcParam #984

OriolAbril opened this issue Jan 5, 2020 · 8 comments · Fixed by #1201

Comments

@OriolAbril
Copy link
Member

OriolAbril commented Jan 5, 2020

Tell us about it

In some cases, users may prefer to visualize the results using 1 indexed values instead of python's 0 indexed ones. Part of #792.

Thoughts on implementation

The summary function already has an index_origin argument, but does not use the rcParam value as default. Moreover, the labels used for plots should also be updated to allow 1 indexed visualization (they all call make_label and selection_to_string).

#981 is also related to this.

@Shashankjain12
Copy link
Contributor

@OriolAbril Can I work on this issue?

@OriolAbril
Copy link
Member Author

@Shashankjain12 Definitely!

@ahartikainen
Copy link
Contributor

I'm not sure if make_label can be fixed currently. And summary has been fixed.

The selection function still needs this.

@Shashankjain12
Copy link
Contributor

So, @ahartikainen is the make_label is fixed can I work on this issue now or is this issue dependant on some certain other function which needs to be fixed first?

@ahartikainen
Copy link
Contributor

You can work to fix .sel method. I think we need to rewrite the whole make_label function so you don't need to worry about it yet.

@Shashankjain12
Copy link
Contributor

Ok thanks @ahartikainen 👍

@Shashankjain12
Copy link
Contributor

Shashankjain12 commented Feb 1, 2020

So, @OriolAbril and @ahartikainen we need to change summary function index_origin parameter to rc_params data.index_orign default parameter instead of None for now and also change the value of data.index_origin in defaultParams in rcparams to 1 instead of 0 over there

@OriolAbril
Copy link
Member Author

we need to change summary function index_origin parameter to rc_params data.index_orign default parameter instead of None

None is used to indicate the value in rcParams must be used, it used to be hardcoded to 1. This was fixed by @ahartikainen while working on a related issue in summary:

I'm not sure if make_label can be fixed currently. And summary has been fixed.

Therefore summary does not need to be modified.

also change the value of data.index_origin in defaultParams in rcparams to 1 instead of 0 over there

We want to keep 0 as the default, to follow Python indexation, and internally we will always used 0 indexed values. The option to use 1 as index origin is a visualization aid for users analyzing Stan or Julia generated samples. In these cases, their model would use 4 chains: 1,2,3,4 so having ArviZ plots and summaries show the same names as the ones used by their model can reduce a lot of index confusions.

If you feel up to it you can work on the .sel method so that in addition to seeing 1 indexed values, users can use the same values shown in plots to index inference data objects. The idea is to correct the indexes provided by users and convert them to 0 based indexes if necessary to keep ArviZ internally always working with 0 based indexing. I feel that a warning is in order though, as labels used for selection may be strings in which case no correction must be done. Working on indexing in sel is probably not a good first issue.

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 a pull request may close this issue.

3 participants