Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

remove Bokeh deprecation warnings #1657 #1683

Merged

Conversation

vikramnagashoka
Copy link
Contributor

@vikramnagashoka vikramnagashoka commented Apr 29, 2021

Description

Replaced the Dash and Cross functions as they are deprecated with Scatter function from the bokeh.models.glyphs module.

Checklist

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

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.

Thanks for the PR

Copy link
Contributor Author

@vikramnagashoka vikramnagashoka left a comment

Choose a reason for hiding this comment

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

Updated to use marker variable instead of constant value of 'cross'

@OriolAbril
Copy link
Member

You have to make sure that all tests pass. Do you need help interpreting the ci errors?

@vikramnagashoka
Copy link
Contributor Author

Yes please. I see that the error is related to pylint. The output from this task says score is 10/10. Also when I navigated to the azure pipeline it said there were other jobs which failed with same error.

@OriolAbril
Copy link
Member

You have to take a look at the messages at https://dev.azure.com/ArviZ/ArviZ/_build/results?buildId=4342&view=logs&j=af9a1d3a-ff8f-5483-6823-86518c773a9b&t=91d5999d-c3e7-502d-15b0-1abfa51eab6f&l=14, I never really understood the score out of 10 that gets assigned. What's important is to fix all the messages, even if it says 10, if there are error messages, there are things wrong and CI will therefore fail. The errors are related to lines too long (running black should fix this) and to extra code that is no longer needed with the Scatter instead of marker_fun change

@OriolAbril
Copy link
Member

It looks like you have modified many unrelated files and now black is complaining. Could it be that you ran black but you have an old version of black installed?

@vikramnagashoka
Copy link
Contributor Author

@OriolAbril not sure what you mean by unrelated files. All the files that I changed had either the Dash or Cross functions which I replaced with Scatter. Can you please let me know what I'm missing here ?

The black version I'm using is as below.
black, version 21.4b2

Can you please help me resolve this ?

@OriolAbril
Copy link
Member

I must have been looking at a wrong commit or misread the filenames, sorry. CI is currently failing at the black step: https://dev.azure.com/ArviZ/ArviZ/_build/results?buildId=4344&view=logs&j=af9a1d3a-ff8f-5483-6823-86518c773a9b&t=6a1cfc46-f4a4-5e3c-e2fb-18ea29f733bf&l=14. It looks like (https://dev.azure.com/ArviZ/ArviZ/_build/results?buildId=4344&view=logs&j=af9a1d3a-ff8f-5483-6823-86518c773a9b&t=0aaa11b3-1b3c-5ed3-0c54-8fd8e63dd2e8&l=25) you have the same version installed as the one running on CI, so you should run black and commit the changes to fix this. The command to run black on ArviZ is on the PR checklist: https://github.com/arviz-devs/arviz/blob/main/CONTRIBUTING.md#pull-request-checklist

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #1683 (e1d65e5) into main (e8e9f79) will increase coverage by 0.00%.
The diff coverage is 94.11%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1683   +/-   ##
=======================================
  Coverage   90.84%   90.84%           
=======================================
  Files         108      108           
  Lines       11825    11828    +3     
=======================================
+ Hits        10742    10745    +3     
  Misses       1083     1083           
Impacted Files Coverage Δ
arviz/plots/backends/bokeh/kdeplot.py 96.79% <83.33%> (+0.02%) ⬆️
arviz/plots/backends/bokeh/elpdplot.py 86.51% <100.00%> (-0.15%) ⬇️
arviz/plots/backends/bokeh/essplot.py 98.52% <100.00%> (+0.02%) ⬆️
arviz/plots/backends/bokeh/mcseplot.py 98.59% <100.00%> (+0.02%) ⬆️
arviz/plots/backends/bokeh/traceplot.py 94.53% <100.00%> (+0.03%) ⬆️

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 e8e9f79...e1d65e5. Read the comment docs.

@vikramnagashoka
Copy link
Contributor Author

@OriolAbril thanks for the details. I have pushed changes after running black and now all checks pass. Can you please help me merge the branch ?

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.

Thanks for updating and fixing all the tests, you should add the fixes to the changelog in the "maintenence and fixes" section and we'll merge the PR 😄

If you are worried about the PR not being merged eventually I can already guarantee it will be merged

@OriolAbril OriolAbril merged commit b98aedb into arviz-devs:main Jun 14, 2021
@OriolAbril
Copy link
Member

Thanks @vikramnagashoka

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants