Add type hints to all of src - #483
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
==========================================
+ Coverage 98.75% 98.78% +0.03%
==========================================
Files 17 16 -1
Lines 2723 2628 -95
==========================================
- Hits 2689 2596 -93
+ Misses 34 32 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive type hints to the pyscal codebase. The changes modernize the codebase by adding proper type annotations throughout the source files and adjusting imports to support type checking.
- Adds type hints to function parameters, return types, and variable declarations
- Updates import statements to include type checking dependencies
- Refactors variable names to avoid shadowing built-in types
Reviewed Changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/*.py | Updates imports from pyscal.utils.testing to tests.utils |
| src/pyscal/wateroil.py | Adds type hints, imports matplotlib at top level, adds type: ignore comments for pandas operations |
| src/pyscal/utils/string.py | Adds return type annotation to comment_formatter function |
| src/pyscal/utils/relperm.py | Adds type hints to truncate_zeroness function and pandas operations |
| src/pyscal/utils/monotonicity.py | Adds type: ignore comments for pandas diff operations |
| src/pyscal/utils/interpolation.py | Adds comprehensive type hints and imports numpy.typing |
| src/pyscal/utils/capillarypressure.py | Adds union type hints for pandas Series support |
| src/pyscal/pyscallist.py | Adds type hints to init and getitem methods |
| src/pyscal/pyscalcli.py | Renames parametertable variable to avoid shadowing |
| src/pyscal/plotting.py | Adds TypedDict for kwargs and comprehensive type hints |
| src/pyscal/gaswater.py | Adds type hints and imports matplotlib at top level |
| src/pyscal/gasoil.py | Adds type hints and imports matplotlib at top level |
| src/pyscal/factory.py | Adds comprehensive type hints and fixes variable shadowing |
| pyproject.toml | Updates ruff configuration and adds type checking dependencies |
| docs/make_plots.py | Adds type hints to all function definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
any risk that someone using this API in their script?
There was a problem hiding this comment.
I guess there is a risk for it, although the file docstring says the following:
"""Common functions and mock data for usage in pyscal testing"""
There was a problem hiding this comment.
Of the equinor repos I could only find usage of it in one subscript test.
There was a problem hiding this comment.
Good point. I guess we can just try it out first 👍
8818aa9 to
0200888
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # are there or even correct, therefore we effectively | ||
| # recalculate them | ||
| def sw_fn(swn): | ||
| def sw_fn(swn: pd.Series) -> pd.Series: |
There was a problem hiding this comment.
Just curious, why not numpy array here instead of pandas series?
There was a problem hiding this comment.
I thought it was only used with pandas series internally, but have tried an alternative approach now.
| def plotkrwkrg( | ||
| self, | ||
| mpl_ax=None, | ||
| mpl_ax: Optional[matplotlib.axes.Axes] = None, |
There was a problem hiding this comment.
why not matplotlib.axes.Axes | None = None
There was a problem hiding this comment.
I will have a separate pr to add ruff rule UP that automatically modernizes the typing
|
|
||
|
|
||
| def format_relperm_plot(fig: plt.Figure, **kwargs) -> plt.Figure: | ||
| def format_relperm_plot(fig: plt.Figure, **kwargs: Any) -> plt.Figure: # noqa: ANN401 |
There was a problem hiding this comment.
Can we use Unpack[PlotKwargs] as well here?
There was a problem hiding this comment.
Have refactored this now to avoid Any
alifbe
left a comment
alifbe
left a comment
There was a problem hiding this comment.
Looks good to me. Just some question out of curiosity. But I think ideally we'd like to avoid Any type.
96c3af5 to
c5994a4
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Needs #482