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

Feature/concurrent regression #531

Merged
merged 40 commits into from
Oct 23, 2023
Merged

Feature/concurrent regression #531

merged 40 commits into from
Oct 23, 2023

Conversation

rafa9811
Copy link
Contributor

@rafa9811 rafa9811 commented Mar 31, 2023

Functional response with multivariate and concurrent covariates. Some fixes in linear regression with scalar response. Tests and examples.

@rafa9811 rafa9811 requested a review from vnmabus March 31, 2023 14:48
penalty_matrix=penalty_matrix,
)
inner_products_list = [
c.regression_matrix(x, y) # type: ignore[arg-type]

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Unused "type: ignore" comment

return nquad_vec(
integrand,
domain_range,
) # type: ignore[no-any-return]

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Unused "type: ignore" comment

@@ -56,13 +57,17 @@ class LinearRegression(
NDArrayFloat,
],
):
r"""Linear regression with multivariate response.
r"""Linear regression with multivariate and functional response.

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D204 1 blank line required after class docstring

skfda/ml/regression/_linear_regression.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (fe6a369) 86.17% compared to head (a020784) 86.27%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #531      +/-   ##
===========================================
+ Coverage    86.17%   86.27%   +0.09%     
===========================================
  Files          149      149              
  Lines        11764    11912     +148     
===========================================
+ Hits         10138    10277     +139     
- Misses        1626     1635       +9     
Files Coverage Δ
skfda/ml/regression/_coefficients.py 90.38% <100.00%> (-1.62%) ⬇️
skfda/tests/test_regression.py 99.67% <100.00%> (+0.07%) ⬆️
skfda/ml/regression/_linear_regression.py 96.70% <95.41%> (-1.38%) ⬇️
skfda/misc/_math.py 81.29% <53.84%> (-3.21%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

skfda/ml/regression/_linear_regression.py Outdated Show resolved Hide resolved
skfda/ml/regression/_linear_regression.py Outdated Show resolved Hide resolved
skfda/ml/regression/_linear_regression.py Outdated Show resolved Hide resolved
skfda/ml/regression/_linear_regression.py Outdated Show resolved Hide resolved
@rafa9811 rafa9811 marked this pull request as ready for review April 15, 2023 20:32
"""Test that the number of response samples and explanatory samples
are not different """
def test_error_y_X_samples_different(self) -> None: # noqa: N802
"""Number of response samples and explanatory samples are not different.

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (80 > 79 characters)

np.testing.assert_equal(y_basis, pred[1].basis)

def test_error_y_X_samples_different(self) -> None: # noqa: N802
"""Number of response samples and explanatory samples are not different.

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (80 > 79 characters)

@@ -157,8 +157,12 @@ def coefficient_info_from_covariate(
def _coefficient_info_from_covariate_ndarray( # type: ignore[misc]
X: NDArrayFloat,
y: NDArrayFloat,
basis: Basis = None,

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Incompatible default for argument "basis" (default has type "None", argument has type "Basis") [assignment]

@@ -157,8 +157,12 @@ def coefficient_info_from_covariate(
def _coefficient_info_from_covariate_ndarray( # type: ignore[misc]
X: NDArrayFloat,
y: NDArrayFloat,
basis: Basis = None,

Choose a reason for hiding this comment

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

📝 [mypy] reported by reviewdog 🐶
PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True

@@ -157,8 +157,12 @@ def coefficient_info_from_covariate(
def _coefficient_info_from_covariate_ndarray( # type: ignore[misc]
X: NDArrayFloat,
y: NDArrayFloat,
basis: Basis = None,

Choose a reason for hiding this comment

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

📝 [mypy] reported by reviewdog 🐶
Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase


Xt = self._make_transpose(X_new)

for i, basis_i in enumerate(self.coef_basis):

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 to "enumerate" has incompatible type "Sequence[Basis | None] | None"; expected "Iterable[Basis | None]" [arg-type]

Xt = self._make_transpose(X_new)

for i, basis_i in enumerate(self.coef_basis):
coef_lengths.append(basis_i.n_basis)

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Item "None" of "Basis | None" has no attribute "n_basis" [union-attr]

beta_coef_compare,
atol=0.01,
)
np.testing.assert_equal(funct_reg.coef_basis[0], y_basis)

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Value of type "Sequence[Basis | None] | None" is not indexable [index]

beta_coef_compare,
atol=0.01,
)
np.testing.assert_equal(funct_reg.coef_basis[0], y_basis)

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Value of type "Sequence[Basis | None] | None" is not indexable [index]

np.testing.assert_allclose(
funct_reg.basis_coefs[3].ravel(), beta_pacific_R, atol=0.001,
)
np.testing.assert_equal(funct_reg.coef_basis[0], y_fd.basis)

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
Value of type "Sequence[Basis | None] | None" is not indexable [index]

df = pd.DataFrame(cov_dict)

beta_coef_compare = [
[[-0.463, 3.250]],

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
List item 0 has incompatible type "list[float]"; expected "float" [list-item]


beta_coef_compare = [
[[-0.463, 3.250]],
[[12.509, -6.275]],

Choose a reason for hiding this comment

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

🚫 [mypy] reported by reviewdog 🐶
List item 0 has incompatible type "list[float]"; expected "float" [list-item]

],
axis=0,
)
""" if self.functional_response:

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS428 Found statement that has no effect


result += self.intercept_
#TODO: MIRAR ESTO BIEN

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E265 block comment should start with '# '


result += self.intercept_
#TODO: MIRAR ESTO BIEN

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
W291 trailing whitespace

vnmabus
vnmabus previously approved these changes Oct 20, 2023
Copy link
Member

@vnmabus vnmabus left a comment

Choose a reason for hiding this comment

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

I think the better approach with this PR is to merge it, as the functionality works, and then have a look to the code myself, in order to have a global vision and simplify things.

examples/plot_functional_regression.py Outdated Show resolved Hide resolved
examples/plot_functional_regression.py Outdated Show resolved Hide resolved
examples/plot_functional_regression.py Outdated Show resolved Hide resolved
@@ -9,6 +9,7 @@
from skfda.datasets import make_gaussian_process
from skfda.misc.covariances import Gaussian
from skfda.representation.basis import (
FDataBasis,
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

class TestFunctionalLinearRegression(unittest.TestCase):
"""Tests for linear regression with functional response."""

def test_multivariate_covariates_1(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Aren't there better names?

@@ -229,6 +304,7 @@ def __init__(
self.coef_basis = coef_basis
self.fit_intercept = fit_intercept
self.regularization = regularization
self.y_basis = None
Copy link
Member

Choose a reason for hiding this comment

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

Why do you add this here?

f1 = arg1(f_args)[:, 0, :]
f2 = arg2(f_args)[:, 0, :]
try:
f1 = arg1(f_args)[:, 0, :]
Copy link
Member

Choose a reason for hiding this comment

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

What are you attempting here?

"""Tests for linear regression with functional response."""

def test_multivariate_covariates_constant_basic(self) -> None:
"""

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D200 One-line docstring should fit on one line with quotes

FDataBasis(
basis=y_basis,
coefficients=[[1]],
)

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C812 missing trailing comma

)

def test_multivariate_covariates_monomial_basic(self) -> None:
"""

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D200 One-line docstring should fit on one line with quotes

# Currently broken.

# _test_linear_regression_common(
# X_train=X_train.to_numpy(),

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code


# _test_linear_regression_common(
# X_train=X_train.to_numpy(),
# y_train=y_train,

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code

# _test_linear_regression_common(
# X_train=X_train.to_numpy(),
# y_train=y_train,
# expected_coefs=expected_coefs,

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code

# X_train=X_train.to_numpy(),
# y_train=y_train,
# expected_coefs=expected_coefs,
# X_test=X_test.to_numpy(),

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code

# y_train=y_train,
# expected_coefs=expected_coefs,
# X_test=X_test.to_numpy(),
# y_test=y_test,

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code

# expected_coefs=expected_coefs,
# X_test=X_test.to_numpy(),
# y_test=y_test,
# )

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E800 Found commented out code

@@ -56,13 +67,17 @@ class LinearRegression(
NDArrayFloat,
],
):
r"""Linear regression with multivariate response.
r"""Linear regression with multivariate and functional response.

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D412 No blank lines allowed between a section header and its content

elif y_basis is None:
return x_basis
else:
return y_basis

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS503 Found useless returning else statement

linear_regression.basis_coefs[0].ravel(), beta_const_R, atol=0.001,
)
np.testing.assert_allclose(
linear_regression.basis_coefs[1].ravel(), beta_atlantic_R, atol=0.001,

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (82 > 79 characters)

linear_regression.basis_coefs[1].ravel(), beta_atlantic_R, atol=0.001,
)
np.testing.assert_allclose(
linear_regression.basis_coefs[2].ravel(), beta_continental_R, atol=0.001,

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (85 > 79 characters)

linear_regression.basis_coefs[2].ravel(), beta_continental_R, atol=0.001,
)
np.testing.assert_allclose(
linear_regression.basis_coefs[3].ravel(), beta_pacific_R, atol=0.001,

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E501 line too long (81 > 79 characters)

@vnmabus vnmabus merged commit 9ff930d into develop Oct 23, 2023
16 of 17 checks passed
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