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

[WIP] Changed the stats scale as default to None and made changes accordingly as suggested as per the issue #985 #993

Merged
merged 7 commits into from
Jan 20, 2020

Conversation

Shashankjain12
Copy link
Contributor

@Shashankjain12 Shashankjain12 commented Jan 13, 2020

fixes #985

@OriolAbril
Copy link
Member

Hi @Shashankjain12 If you enter to the build logs you will see that only one of the 5 jobs is failing, the one called lint. This is due to the code formatting not following the black code style, this should be solved by black arviz/ examples/ (it's the 3rd point starting from the end in the pull request checklist).

As to solving the issue, you have now solved the issue only partially. compare will now use the value in rcParams, however, there are still functions like loo, waic and elpd_plot that do not yet use the value in rcParams.

@OriolAbril
Copy link
Member

It looks like you have some issues with your black installation. What does black --version print?

@Shashankjain12
Copy link
Contributor Author

@OriolAbril do we need to create test cases as well manually I guess I had not created any maybe that's the problem?

@Shashankjain12
Copy link
Contributor Author

And black, version is 18.9b0

@ahartikainen
Copy link
Contributor

I think you need to update to latest version.

@Shashankjain12
Copy link
Contributor Author

@ahartikainen even after updating black to 19.10b0 no file is changed at all

@OriolAbril
Copy link
Member

@Shashankjain12 Somehow when you ran black, 32 extra files were reformatted (this is why now it says that this PR modifies 33 files), and they were reformatted in a way that the files cannot be parsed in python 3.5 (hence the build failure).

Somehow an old version of black is formatting the code, could there be 2 different installations? Otherwise you can try to code from within docker which will have the correct packages. We have a guide here that should help you.

@Shashankjain12
Copy link
Contributor Author

So @OriolAbril do i need to delete my repo and try that again in order to contribute to this?

@OriolAbril
Copy link
Member

You should not have to delete the whole repository, it looks like it only has to do with black.

Try uninstalling and reinstalling again just in case.

And if it does not work, using docker should allow to code from your computer on the same repo you already have but from within a container that has different programs available (inside which black should work properly)

@ahartikainen
Copy link
Contributor

Hi @Shashankjain12 how have you installed your python? Are you using conda?

For example I use this workflow (with miniconda (+ cmder on Windows)) --> Use conda environment

https://pystan.readthedocs.io/en/latest/installation_beginner.html
https://pystan.readthedocs.io/en/latest/windows.html (<< conda stuff works for other OS too)

https://docs.conda.io/en/latest/miniconda.html <<- I use local folder e.g. "/home/miniconda3" and I add the conda on the PATH at installation step
https://cmder.net/

@OriolAbril
Copy link
Member

@Shashankjain12 The changes in stats.py look great. Only plot_elpd needs to be updated now.

@Shashankjain12
Copy link
Contributor Author

what is plot_elpd @OriolAbril ?

@OriolAbril
Copy link
Member

plot_elpd

@Shashankjain12
Copy link
Contributor Author

Ok sure thanks I will update that 👍

@Shashankjain12
Copy link
Contributor Author

Done @OriolAbril 👍

@OriolAbril OriolAbril merged commit 1f4878f into arviz-devs:master Jan 20, 2020
percygautam pushed a commit to percygautam/arviz that referenced this pull request Jan 21, 2020
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.

Integrate ic_scale rcParam
3 participants