Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ classifiers = [
dependencies = ["numpy>=2.0.0",
"scikit-learn>=1.5.0",
"scipy>=1.13.0",
"array-api-compat",
"torch>=2.12.0,<=2.14.0",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For GFDL the max torch<=2.14.0 may not be necessary. It is on our cluster machines that I run into issues with cuda drivers not compatible with latest torch versions.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, unlikely to be a real constraint--we shouldn't have to maintain an upper bound at all. I added a more detailed comment about where these deps might live rather than dependencies. Since we're a small project, array-api-compat might be "ok" to leave here, though might make sense to pin its version.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think any upstream example projects that I know of add these as runtime dependencies in this way. Probably best to remove and install them in the ways that match the approaches used upstream (i.e., sklearn/scipy, etc.). array-api-compat might be "ok" to leave, since we're a small project.

We can't officially turn the functionality on anyway until SciPy/sklearn turn their array API functionality on, so some manual installing on the part of the user is likely reasonable for now, but torch is something the user would just normally have to install on their own if they want to use it. And the torch binary is massive so pulling that in by default would be wasteful except in CI jobs that genuinely need it. Perhaps torch as a dependency group entry for developers for now. SciPy does have the test-array-types dependency group (https://github.com/scipy/scipy/blob/main/pyproject.toml#L131)--maybe we could use something similar for now.

I also know that SciPy uses git submodules for some of this:

 e1d4eed1389f1d93318ec855730488db48475320 subprojects/array_api_compat (1.14-36-ge1d4eed)
 dc9e6b59061d8a116f90dcaf83c6d1878496e5ae subprojects/array_api_extra (v0.11.1)

we could consider something like that I suppose. Not sure what is best for us yet--we're a bit smaller so may have less constraints, but still pulling in torch automatically seem undesirable. array-api-compat may be "ok" to leave as a normal dep perhaps, since we're "small."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like sklearn vendors some of the deps -- see their infrastructure at: sklearn/externals/array_api_compat. That's probably because adding dependencies is prohibitively expensive for them/their users. We probably don't want to do that, but we may want to do one thing that is similar--pinning our version of that lib and only updating the compatibility layer as needed, because when the array API standard changes, things may change/start to fail, so we probably want to be explicit about changes that adopt newer versions of the array API standard.

"packaging>=24.0"]

[project.urls]
Expand Down
82 changes: 73 additions & 9 deletions src/gfdl/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Estimators for gradient free deep learning.
"""

import array_api_compat
import numpy as np
import scipy
from scipy.special import logsumexp
Expand Down Expand Up @@ -44,12 +45,16 @@ def __init__(
self.rtol = rtol

def fit(self, X, Y):
xp = array_api_compat.get_namespace(X)
X = _ensure_float_X(xp, X)
Y = _check_and_convert_array(X, Y)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Have you actually checked how sklearn does all this stuff? That was the main task assigned really--avoid reinventing the wheel and adopt their approach, especially for simplification of the process of transplanting over to Emma's sklearn PR. Looking through their PR list with a search for "array API" one can learn quite a lot about how they are doing things.

If I look at their LogisticRegression class for example, the fit() method uses this sequence:

        xp, _, device = get_namespace_and_device(X)
        sample_weight = move_to(sample_weight, xp=xp, device=device)
        xp_y, _ = get_namespace(y)

see: https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/linear_model/_logistic.py#L1492-L1494

I can't think of a reason to "roll our own" approach when a large team of folks, including people on the actual standards committee, have vetted upstream approaches.


# Assumption : X, Y have been pre-processed.
# X shape: (n_samples, n_features)
# Y shape: (n_samples, n_classes-1)
if self.reg_alpha is not None and self.reg_alpha < 0.0:
raise ValueError("Negative reg_alpha. Expected range : None or [0.0, inf).")
hidden_layer_sizes = np.asarray(self.hidden_layer_sizes)
hidden_layer_sizes = xp.asarray(self.hidden_layer_sizes)
if hidden_layer_sizes.min() < 1:
raise ValueError("hidden_layer_sizes must be > 0, "
f"got {hidden_layer_sizes}")
Expand All @@ -65,22 +70,34 @@ def fit(self, X, Y):
rng = self.get_generator(self.seed)

self.W_.append(
_check_and_convert_array(
X,
self._weight_mode(
self._N, hidden_layer_sizes[0], rng=self.get_generator(self.seed)
)
),
)
)
self.b_.append(
_check_and_convert_array(
X,
self._weight_mode(1, hidden_layer_sizes[0], rng=rng)
.reshape(-1)
.reshape(-1),
)
)
for i, layer in enumerate(hidden_layer_sizes[1:]):
# (n_hidden, n_features)
self.W_.append(
self._weight_mode(hidden_layer_sizes[i], layer, rng=rng,)
_check_and_convert_array(
X,
self._weight_mode(hidden_layer_sizes[i], layer, rng=rng,),
)
)
# (n_hidden,)
self.b_.append(
self._weight_mode(1, layer, rng=rng,).reshape(-1)
_check_and_convert_array(
X,
self._weight_mode(1, layer, rng=rng,).reshape(-1),
)
)

# hypothesis space shape: (n_layers,)
Expand All @@ -95,15 +112,15 @@ def fit(self, X, Y):
# or (n_samples, sum_hidden)
if self.direct_links:
Hs.append(X)
D = np.hstack(Hs)
D = xp.concat(Hs, axis=1)

# beta shape: (sum_hidden+n_features, n_classes-1)
# or (sum_hidden, n_classes-1)

# If reg_alpha is None, use direct solve using
# MoorePenrose Pseudo-Inverse, otherwise use ridge regularized form.
if self.reg_alpha is None:
self.coeff_ = np.linalg.pinv(D, rtol=self.rtol) @ Y
self.coeff_ = xp.linalg.pinv(D, rtol=self.rtol) @ Y

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This particular invocation has issues with integer arrays. I believe xp.linalg.pinv in some or all namespaces is only implemented for svd, which has an finfo() invocation. The finfo() invocation fails when given an integer array.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be helpful to refer to the standard itself when discussing issues. For example, if we look at: https://data-apis.org/array-api/latest/extensions/generated/array_api.linalg.pinv.html#pinv

It indicates:

x (array) – input array having shape (..., M, N) and whose innermost two dimensions form MxN matrices. Should have a floating-point data type.

So, yes, the input must be of a floating point dtype. git grep -E -i "xp.linalg.pinv" does show some example usages that might be helpful inside the sklearn source. They do seem to occasionally diverge when NumPy is used vs. not.

else:
ridge = Ridge(alpha=self.reg_alpha, fit_intercept=False)
ridge.fit(D, Y)
Expand Down Expand Up @@ -229,17 +246,24 @@ def partial_fit(self, X, y):

def predict(self, X):
check_is_fitted(self)
xp = array_api_compat.get_namespace(X)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here again, check sklearn source may be helpful.

The predict_proba of their LogisticRegression uses check_same_namespace(X, self, attribute="coef_", method="predict_proba") for example.


Hs = []
H_prev = X
for W, b in zip(self.W_, self.b_, strict=False):
W = _check_and_convert_array(X, W)
b = _check_and_convert_array(X, b)
Z = H_prev @ W.T + b # (n, m)
H_prev = self._activation_fn(Z)
Hs.append(H_prev)

if self.direct_links:
Hs.append(X)
D = np.hstack(Hs)
out = D @ self.coeff_
D = xp.concat(Hs, axis=1)
out = D @ _check_and_convert_array(
X,
self.coeff_
)

return out

Expand Down Expand Up @@ -1266,3 +1290,43 @@ def predict(self, X):
check_is_fitted(self)
X = validate_data(self, X, reset=False)
return super().predict(X)


def _check_and_convert_array(X1, X2):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this and the function below should be replaced with what sklearn uses for similar needs, which was one of the main objectives of the task--figure out how they handle the array API support and just do that instead of reinventing the wheel; this will also make it easier to transplant things over to Emma's sklearn PR.

sklearn also allows different devices in some cases... I think scoping would be useful--we don't really need device handling for CPU support and it will be hard to review if you try to do everything at once, but certainly avoid AI drafted utility functions and instead used vetted utility code from upstream

"""Convert second array to namespace, device, dtype of first if not already"""
Comment on lines +1295 to +1296

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Scaffold of this function written by AI. Reviewed and edited by human.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can use sklearn's move_to(*arrays, xp, device) instead of this function. See https://github.com/scikit-learn/scikit-learn/blob/4358542a1bd39af2e7e1cbace8b9fed067473c5b/sklearn/utils/_array_api.py#L523

xp = array_api_compat.get_namespace(X1)
kwargs = {}

if hasattr(X1, "dtype") and getattr(X2, "dtype", None) != X1.dtype:
kwargs["dtype"] = X1.dtype

x1_device = getattr(X1, "device", None)
x2_device = getattr(X2, "device", None)

if x1_device is not None and x1_device != x2_device:
kwargs["device"] = x1_device

if kwargs:
return xp.asarray(X2, **kwargs)

return X2


def _ensure_float_X(xp, X):
"""Make design matrix floating point numbers"""
Comment on lines +1315 to +1316

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Scaffold of this function written by AI. Reviewed and edited by human.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's just use upstream shims for this--we don't need to have AI draft this stuff when it has been battle tested upstream in SciPy and sklearn. I know SciPy has a utility to force floating for array API, etc.

if xp.isdtype(X.dtype, "real floating"):
return X

device = getattr(X, "device", None)
device_str = str(device).lower()

if "cuda" in device_str or "gpu" in device_str or "tpu" in device_str:
dtype = xp.float32
else:
dtype = xp.float64

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this doesn't look right--some (many) CUDA devices support float64

what I was really looking for here is a CPU-only, but very tight, implementation of the array API support approach used by sklearn since they already have battle tested utilities.


kwargs = {"dtype": dtype}
if device is not None:
kwargs["device"] = device

return xp.asarray(X, **kwargs)
205 changes: 205 additions & 0 deletions src/gfdl/tests/test_regression.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import os

import array_api_compat
import numpy as np
import pytest
import torch

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We shouldn't unconditionally import torch--it isn't a mandatory dependency--only optional. Main part of the task was to sort out how sklearn does this kind of thing and mimic it.

There are excellent upstream docs about this, like https://scipy.github.io/devdocs/dev/api-dev/array_api.html#adding-tests.

from numpy.testing import assert_allclose
from sklearn import config_context
from sklearn.base import clone
from sklearn.datasets import fetch_openml, make_regression
from sklearn.metrics import r2_score
Expand Down Expand Up @@ -257,3 +263,202 @@ def test_preserve_class_inputs():
for k, v in actual.items():
assert v == expected[k]
assert isinstance(v, type(expected[k]))


@pytest.mark.parametrize(
"reg_alpha, n_features, hidden_layer_sizes",
[
(1e-1, 40, (100,)),
(1e-1, 400, (100,)),
(2, 40, (100,)),
(2, 400, (100,)),
(None, 40, (100,)),
(None, 400, (100,)),
(1e-1, 40, (100, 100,)),
(1e-1, 400, (100, 100,)),
(2, 40, (100, 100,)),
(2, 400, (100, 100,)),
(None, 40, (100, 100,)),
(None, 400, (100, 100,)),
]
)
def test_torch_matches_numpy(reg_alpha,
n_features,
hidden_layer_sizes,
):
"""NumPy and Torch predictions should be close up to atol"""
os.environ["SCIPY_ARRAY_API"] = "1"
estimator = GFDLRegressor
report = r2_score
Comment on lines +291 to +292

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These are written to potentially be compatible with a GFDLClassifier test function or other scoring metrics in the context or a parametrization decorator mark.

rng = np.random.default_rng(seed=42)
acc_np_s = []
acc_torch_s = []
for _ in range(10):
random_state = rng.integers(low=0, high=100, size=1)[0]
X_np, y_np = make_regression(
n_samples=10_000,
n_features=n_features,
n_informative=int(n_features / 10),
n_targets=1,
random_state=random_state,
)
X_torch = torch.asarray(X_np, device="cpu",)
y_torch = torch.asarray(y_np, device="cpu",)
with config_context(array_api_dispatch=True):
model = estimator(reg_alpha=reg_alpha,
hidden_layer_sizes=hidden_layer_sizes,
seed=random_state,
)
model.fit(X_np, y_np)
y_pred = model.predict(X_torch)
acc_torch = report(
y_torch,
y_pred,
)
acc_torch_s.append(acc_torch)
# test against numpy which is current usage
with config_context(array_api_dispatch=True):
model = estimator(reg_alpha=reg_alpha,
hidden_layer_sizes=hidden_layer_sizes,
seed=random_state,
)
model.fit(X_np, y_np)
y_pred = model.predict(X_np)
acc_np = report(
y_np,
y_pred,
)
acc_np_s.append(acc_np)
assert_allclose(acc_torch_s, acc_np_s, atol=1e-3)


@pytest.mark.parametrize(
"namespace, fit_dtype, predict_dtype",
[
(np, np.float64, np.float32),
(torch, torch.float64, torch.float32),
(np, np.float32, np.float64),
(torch, torch.float32, torch.float64),
Comment on lines +338 to +341

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is written for stability on floating point precision 32 and 64, for potential downstream PRs on accelerator devices.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't look quite right--should be handled by upstream testing utilities/approaches. People have already thought carefully about this stuff and I don't want us to have to rehash all those discussions. Would become a nightmare to maintain this with 4-5 more namespaces.

]
)
def test_predictor_context(namespace,
fit_dtype,
predict_dtype,
):
Comment on lines +344 to +347

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This function refers to a design choice: that the .predict() calculation should be done on the device that the user specifies, i.e., the design matrix they put in. I expect reviewer discussion on this design choice. An alternative would be to make the .predict() calculations on the device of X used in .fit().

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should include a clear discussion of what sklearn does in this scenario--the main point of the task was to identify what they do and match that, rather than rehashing discussions and utilities that have already been battle tested upstream.

# ValueError thrown .predict has
# different namespace, dtype
# from its arguments
os.environ["SCIPY_ARRAY_API"] = "1"
estimator = GFDLRegressor
device = "cpu"

X, y = make_regression(
n_samples=1_000,
n_features=10,
n_informative=4,
n_targets=1,
random_state=42,
)
X1 = namespace.asarray(X, dtype=fit_dtype, device=device)
X2 = namespace.asarray(X, dtype=predict_dtype, device=device)
y = namespace.asarray(y, dtype=fit_dtype, device=device)
with config_context(array_api_dispatch=True):
y_pred = estimator().fit(X1, y).predict(X2)

xp_ypred = array_api_compat.get_namespace(y_pred)
xp_X2 = array_api_compat.get_namespace(X2)
if xp_ypred != xp_X2:
raise ValueError(
".predict output and input are not in the same namespace"
)
if predict_dtype != y_pred.dtype:
raise ValueError(
".predict output and input are not the same dtype"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we don't typically raise ValueError in tests; a lot of this looks kind of weird--you'd typically just do a plain assertion, but actually dtype passthrough/preservation is already built-in to upstream utilities so we really shouldn't spend much time reviewing or discussing things that have already been battle tested upstream for array API testing/support

)


@pytest.mark.parametrize(
"namespace, X_dtype, y_dtype",
[
(np, np.float64, np.int64),
(np, np.int64, np.float64),
(torch, torch.float64, torch.int64),
]
)
def test_int_array_api(namespace,
X_dtype,
y_dtype,
):
"""Integer arrays shall be handled gracefully"""
Comment on lines +388 to +392

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I often ran into issues with an sklearn test with input integer arrays. This was the motivation for the private utility function _ensure_float_X, and this test is meant to confirm that that works.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LogisticRegression does this in their validate_data calls with dtype=[xp.float64, xp.float32].

os.environ["SCIPY_ARRAY_API"] = "1"
estimator = GFDLRegressor

X, y = make_regression(
n_samples=1_000,
n_features=10,
n_informative=4,
n_targets=1,
random_state=42,
)
X = namespace.asarray(X, dtype=X_dtype, device="cpu")
y = namespace.asarray(y, dtype=y_dtype, device="cpu")
with config_context(array_api_dispatch=True):
estimator().fit(X, y)


@pytest.mark.parametrize(
"namespace, X_dtype, y_dtype",
[
(np, np.float64, np.float64),
(np, np.float32, np.float32),
(torch, torch.float64, torch.float64),
(torch, torch.float32, torch.float32),
(torch, torch.float64, torch.float32),
]
)
def test_fit_attr_context(namespace,
X_dtype,
y_dtype,
):
Comment on lines +419 to +422

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I ran into some issues where the weights, generated as NumPy arrays would in the wrong namespace, device, or dtype as X or Y. This was one of the motivations for the private utility function _check_and_convert_array. This test is meant to test that the fitted attributes are saved as the same namespace, device, and dtype of X.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the main point of the task was to identify what utilities/approaches sklearn uses to check these things, and use those, rather than rehashing all of that here.

In the current sklearn source I see test_logistic_regression_array_api_compliance which uses yield_namespace_device_dtype_combinations() and _array_api_for_tests()--probably not much point in me spending time here until we're matching what they've thought carefully about upstream, etc.

"""Fitted attributes shall be same as design matrix"""
os.environ["SCIPY_ARRAY_API"] = "1"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is obviously not correct--it would bleed into other tests in our testsuite, which we don't want.

We'll need a way to turn this on/off globally for the testsuite, along with a way to select which array backends we want to test at any given time. The user/developer/CI testing could just set this and other flags externally depending on what they are trying to test.

This stuff has been hashed out upstream, so let's not reinvent the wheel.

estimator = GFDLRegressor
device = "cpu"

X, y = make_regression(
n_samples=1_000,
n_features=10,
n_informative=4,
n_targets=1,
random_state=42,
)
X = namespace.asarray(X, dtype=X_dtype, device=device)
y = namespace.asarray(y, dtype=y_dtype, device=device)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

note that CuPy asarray() doesn't suport device=; this stuff has all been thought through upstream though.. see my other comments

with config_context(array_api_dispatch=True):
model = estimator().fit(X, y)

dtypes = []
devices = []
spaces = []
W_ = model.W_
b_ = model.b_
for w, b in zip(W_, b_, strict=False):
dtypes.extend((b.dtype, w.dtype))
devices.extend((b.device, w.device))
spaces.extend(
(array_api_compat.get_namespace(w),
array_api_compat.get_namespace(b),)
)

coeff_ = model.coeff_
dtypes.append(coeff_.dtype)
devices.append(coeff_.device)
spaces.append(
array_api_compat.get_namespace(coeff_)
)

assert all(dtype == X.dtype for dtype in dtypes)
assert all(device == X.device for device in devices)

X_space = array_api_compat.get_namespace(X)
assert all(space == X_space for space in spaces)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A lot of this stuff is handled by upstream utilities--that's probably the main part of the assigned task--mimic how sklearn handle this stuff (device checking, namespace checking, dtype checking) rather than reinventing the wheel.

You can search through their PR list for "array API" and so on, and use git grep on their codebase to poke around.

sklearn has check_array_api_same_namespace for example. That or something similar might suit here.

We'll also want a way to be able to run the testsuite with/without array API backend checking, to specify which backends we want to check (all vs. just torch, etc.).

Loading