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] fix mypy errors #1586

Merged
merged 2 commits into from
Mar 1, 2021
Merged

Conversation

utkarsh-maheshwari
Copy link
Contributor

@utkarsh-maheshwari utkarsh-maheshwari commented Feb 26, 2021

Description

Fixed mypy errors mentioned in #1528 (comment) by adding type hints to var_names and latex_elements. Addresses #1581

Checklist

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

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #1586 (c9db950) into main (a9e5812) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1586   +/-   ##
=======================================
  Coverage   90.05%   90.05%           
=======================================
  Files         108      108           
  Lines       11584    11584           
=======================================
  Hits        10432    10432           
  Misses       1152     1152           
Impacted Files Coverage Δ
arviz/plots/traceplot.py 95.55% <100.00%> (ø)

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 a9e5812...1383ea5. Read the comment docs.

@utkarsh-maheshwari
Copy link
Contributor Author

Checks are passed but how should I check if the mypy errors have been resolved?

@OriolAbril
Copy link
Member

There should only be a mypy.ini file that covers the whole package. Both the mypy.ini file and pinning mypy 0.790 will be done by #1528 as a temporal fix. Temporal because they are not fixing the issue, they are only ignoring it and sliencing it for a while so we can keep ahead and start using typing_copilot as soon as possible.

To fix the errors you'll have to edit the actual arviz library, nothing should be modified in the examples folder. The first errors listed in the other PR for example are complaining that var_names is a tuple instead of a list. This is a false positive, the mistake is in the type hints that the function (plot_trace in this case) has. This is the line that defines the type hint in question:

var_names: Optional[List[str]] = None,

List is not the correct type hint, a tuple or a numpy array are perfectly valid options. Thus, the type hint should use Sequence instead of list.

To run mypy I believe you have to run mypy -p arviz from the base folder, but if I understand correctly, it will only work if you have the mypy.ini defined in #1528.

@utkarsh-maheshwari
Copy link
Contributor Author

utkarsh-maheshwari commented Feb 27, 2021

Okay. So I have changed the type hint for var_names. First six have been resolved now.

The major doubt that I am having is before changing the type hint to Sequence, I run the command by pinning 2 different versions of mypy
typing_copilot init --overwrite

I can see the 7 errors in case I pin mypy==0.812 but not if I pin mypy==0.790. So, according to #1528 (comment) option 1, if we pin mypy<0.800, do we really need to change the type hint?

@OriolAbril OriolAbril changed the title [WIP] fix numpy errors [WIP] fix mypy errors Feb 27, 2021
@OriolAbril
Copy link
Member

So, according to #1528 (comment) option 1, if we pin mypy<0.800, do we really need to change the type hint?

Yes, because the ideal solution is doing option 2. Option 1 is a temporal fix that allows us to continue working on #1528 and not have to wait until we have time to fix all the issues (as we are all volunteers, it is not clear when we would have time and if it takes us too long maybe by the time we get to do it instead of 7 there are 30 errors).

We are doing similar things with #1578 where we disabled some pylint checks to get CI to pass quickly but we did not fix those issues, or with #1537 where we have temporarily disabled pyjags tests because pyjags requires numpy >=1.20 and tensorflow probability requires numpy <=1.19 because our tfp converter is outdated. Fixing the converter is complicated and depends also on changes on tfp side so we feel it's better to wait for now until we can update the code and fix the root issue, if we updated the code now, we may have to rewrite it again in a few months

@utkarsh-maheshwari
Copy link
Contributor Author

Okay I got it now. Very nicely explained. Thank you!

@utkarsh-maheshwari
Copy link
Contributor Author

Do these changes needed to be listed in changelog?

@OriolAbril
Copy link
Member

I don't think so, thanks! I am realizing that there are many other places where we have inputs type hinted as lists when they should be sequences. I will merge this to go ahead with typing copilot directly with latest mypy but keep the issue open.

@OriolAbril OriolAbril merged commit 83ae035 into arviz-devs:main Mar 1, 2021
@OriolAbril OriolAbril mentioned this pull request Mar 5, 2021
@utkarsh-maheshwari utkarsh-maheshwari deleted the fix-mypy-errors branch May 27, 2021 09:38
utkarsh-maheshwari added a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* Changed type ints for var_names

* fix mypy errors
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.

2 participants