-
Notifications
You must be signed in to change notification settings - Fork 55
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
Occupation measure construction #396
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #396 +/- ##
========================================
Coverage 80.19% 80.20%
========================================
Files 93 93
Lines 7266 7289 +23
========================================
+ Hits 5827 5846 +19
- Misses 1439 1443 +4
Continue to review full report at Codecov.
|
|
||
left, right = domain_range[0] | ||
interval_size = (right - left) / n_intervals | ||
integrated_data = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [mypy] reported by reviewdog 🐶
Need type annotation for 'integrated_data' (hint: "integrated_data: List[] = ...")
def _calculate_curves_occupation_( | ||
curve_y_coordinates: np.ndarray, | ||
curve_x_coordinates: np.ndarray, | ||
interval: Tuple, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [mypy] reported by reviewdog 🐶
Missing type parameters for generic type "Tuple"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Tuple[float, float]
.
@@ -124,12 +130,11 @@ def __init__( | |||
self.extrapolation = extrapolation | |||
self.grid = grid | |||
|
|||
def fit( # noqa: D102 | |||
def fit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [mypy] reported by reviewdog 🐶
Argument 1 of "fit" is incompatible with supertype "TransformerMixin"; supertype defines the argument type as "Input"
@@ -124,12 +130,11 @@ def __init__( | |||
self.extrapolation = extrapolation | |||
self.grid = grid | |||
|
|||
def fit( # noqa: D102 | |||
def fit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 [mypy] reported by reviewdog 🐶
This violates the Liskov substitution principle
@@ -124,12 +130,11 @@ def __init__( | |||
self.extrapolation = extrapolation | |||
self.grid = grid | |||
|
|||
def fit( # noqa: D102 | |||
def fit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 [mypy] reported by reviewdog 🐶
See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
skfda/misc/_math.py
Outdated
integrand = d1 * d2 | ||
integrand = arg1 * arg2 | ||
return integrand.integrate() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pep8] reported by reviewdog 🐶
W293 blank line contains whitespace
@@ -124,12 +130,11 @@ def __init__( | |||
self.extrapolation = extrapolation | |||
self.grid = grid | |||
|
|||
def fit( # noqa: D102 | |||
def fit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method
@@ -140,12 +145,11 @@ def fit( # noqa: D102 | |||
|
|||
return self | |||
|
|||
def transform( # noqa: D102 | |||
def transform( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method
interval: Optional[DomainRange] = None, | ||
) -> NDArrayFloat: | ||
""" | ||
Integration of the FData object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pep8] reported by reviewdog 🐶
DAR003 Incorrect indentation: ~<
*, | ||
interval: Optional[DomainRange] = None, | ||
) -> NDArrayFloat: | ||
"""Examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pep8] reported by reviewdog 🐶
D205 1 blank line required between summary line and description
interval: Optional[DomainRange] = None, | ||
) -> NDArrayFloat: | ||
"""Integration of the FData object. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[pep8] reported by reviewdog 🐶
DAR003 Incorrect indentation: ~<
skfda/exploratory/stats/__init__.py
Outdated
@@ -1,5 +1,9 @@ | |||
from ._fisher_rao import _fisher_rao_warping_mean, fisher_rao_karcher_mean | |||
from ._functional_transformers import local_averages | |||
from ._functional_transformers import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [mypy] reported by reviewdog 🐶
Module 'skfda.exploratory.stats._functional_transformers' has no attribute 'number_up_crossings'
|
||
def occupation_measure( | ||
data: Union[FDataGrid, FDataBasis], | ||
intervals: np.ndarray, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a Sequence
of Tuple[float, float]
?
data: Union[FDataGrid, FDataBasis], | ||
intervals: np.ndarray, | ||
*, | ||
n_points: Union[int, None] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Optional[int]
instead of Union[int, None]
.
intervals: np.ndarray, | ||
*, | ||
n_points: Union[int, None] = None, | ||
) -> np.ndarray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a more specific type if possible, such as NDArrayFloat
.
+ " as an argument for a FDataBasis. Instead None was passed.", | ||
) | ||
|
||
for interval_check in intervals: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be checked in _calculate_curves_occupation_
without the need of an additional loop.
def _calculate_curves_occupation_( | ||
curve_y_coordinates: np.ndarray, | ||
curve_x_coordinates: np.ndarray, | ||
interval: Tuple, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Tuple[float, float]
.
less_than_y2 = curve_y_coordinates <= y2 | ||
inside_interval_bools = greater_than_y1 & less_than_y2 | ||
|
||
# Correct booleans so they are not repeated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to explain more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't mark conversations as solved unless the underlying issue is addressed.
|
||
Args: | ||
data: FDataGrid or FDataBasis where we want to calculate | ||
the occupation measure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent multiline descriptions properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #371