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

DOC add type annotations to responsibleai package #1214

Merged
merged 10 commits into from
Feb 11, 2022

Conversation

romanlutz
Copy link
Contributor

Description

Adding type annotations to the responsibleai package wherever there was anything Optional. Would we want these for the required args as well? Also fixing a few smaller formatting inconsistencies and typos in the process.

Areas changed

npm packages changed:

  • responsibleai/causality
  • responsibleai/core-ui
  • responsibleai/counterfactuals
  • responsibleai/dataset-explorer
  • responsibleai/fairness
  • responsibleai/interpret
  • responsibleai/localization
  • responsibleai/mlchartlib
  • responsibleai/model-assessment

Python packages changed:

  • raiwidgets
  • responsibleai
  • erroranalysis
  • rai_core_flask

Tests

  • No new tests required.
  • New tests for the added feature are part of this PR.
  • I validated the changes manually.

Screenshots (if appropriate):

Documentation:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@romanlutz romanlutz added the documentation Improvements or additions to documentation label Feb 10, 2022
@romanlutz romanlutz requested a review from riedgar-ms February 10, 2022 03:05
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022

Codecov Report

Merging #1214 (29a6886) into main (18d38dd) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1214      +/-   ##
==========================================
+ Coverage   67.12%   67.19%   +0.07%     
==========================================
  Files          91       91              
  Lines        4383     4393      +10     
==========================================
+ Hits         2942     2952      +10     
  Misses       1441     1441              
Flag Coverage Δ
unittests 67.19% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...i/responsibleai/managers/counterfactual_manager.py 94.19% <100.00%> (+0.03%) ⬆️
...i/responsibleai/managers/error_analysis_manager.py 89.88% <100.00%> (+0.11%) ⬆️
...ibleai/responsibleai/managers/explainer_manager.py 68.75% <100.00%> (+0.32%) ⬆️
...leai/responsibleai/modelanalysis/model_analysis.py 94.33% <100.00%> (+0.33%) ⬆️
...sibleai/responsibleai/rai_insights/rai_insights.py 77.87% <100.00%> (+0.06%) ⬆️

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 18d38dd...29a6886. Read the comment docs.

@romanlutz
Copy link
Contributor Author

@gaugup if I'm understanding correctly you mean I should add type annotations for every arg, not just the optional ones. Is that correct?

1 similar comment
@gaugup
Copy link
Contributor

gaugup commented Feb 10, 2022

@gaugup if I'm understanding correctly you mean I should add type annotations for every arg, not just the optional ones. Is that correct?

Type hints are for static checking of the code paths. So a variable being optional or not doesn't matter. We haven't enabled mypy in our builds otherwise you would see these unannotated function variables getting flagged.

@romanlutz
Copy link
Contributor Author

romanlutz commented Feb 10, 2022

@gaugup if I'm understanding correctly you mean I should add type annotations for every arg, not just the optional ones. Is that correct?

Type hints are for static checking of the code paths. So a variable being optional or not doesn't matter. We haven't enabled mypy in our builds otherwise you would see these unannotated function variables getting flagged.

@gaugup Is that a yes? :-)

@imatiach-msft
Copy link
Contributor

oh, yes, you can add type hints to all parameters, certainly not just the optionals ones. So it's a yes.

2 similar comments
@romanlutz romanlutz enabled auto-merge (squash) February 11, 2022 04:06
@romanlutz romanlutz merged commit 742cf99 into main Feb 11, 2022
@romanlutz romanlutz deleted the romanlutz/optional_api_docs branch February 11, 2022 05:20
gaugup pushed a commit that referenced this pull request Feb 27, 2022
* add type annotations

* fix imports

* import ordering fix

* more type annotations

* add Any annotation for model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants