Repository navigation
Conversation
Ashu56376
changed the title
tylerjereddy
left a comment
tylerjereddy
left a comment
There was a problem hiding this comment.
Thanks -- I added some initial review comments. If you used AI/an LLM to prepare any of the work, can you add a brief disclosure to your original PR comment about that, just so we know.
How did you find this project? Do you use RVFL/ELM networks in your work or?
I did confirm that the regression test you added fails before and passes after the source patch. I noted a few small things and of course that the target issue is actually about the regressor. I don't mind if you want to handle the regressor in a separate PR, but let's not consider the target issue fully closed until the testing confirms the regressor is "ok" as well.
| predictions = model.predict(X[:5]) | ||
| assert predictions.shape == (5,) | ||
|
|
||
|
|
There was a problem hiding this comment.
There are some ruff linting failures here and elsewhere. You can find these by running ruff check with ruff version 0.16.8 locally.
| "Classifier", | ||
| [GFDLClassifier, EnsembleGFDLClassifier], | ||
| ) | ||
| def test_scalar_hidden_layer_sizes(Classifier): |
There was a problem hiding this comment.
it is helpful to reference the issue you are writing a test for, i.e., # see gh-136
| 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 = np.atleast_1d(np.asarray(self.hidden_layer_sizes)) |
There was a problem hiding this comment.
I think that, here and elsewhere, you can simplify the call to remove np.asarray() since atleast_1d should call something similar for array coercion under the hood.
| ) | ||
|
|
||
| model = Classifier(hidden_layer_sizes=3, seed=0) | ||
| model.fit(X, y) |
There was a problem hiding this comment.
I suppose if we're being really thorough, we should also test partial_fit()
|
|
||
| @pytest.mark.parametrize( | ||
| "Classifier", | ||
| [GFDLClassifier, EnsembleGFDLClassifier], |
There was a problem hiding this comment.
Note that the target issue concerns GFDLRegressor rather than the classifiers. I don't mind if you want to test/fix them separately (regressor tests go in a separate testing module), but let's not consider the target issue fully closed until the GFDLRegressor is tested for the problem scenario in the matching issue.
tylerjereddy
changed the title
Thanks for the thorough review and for confirming that the regression test indeed fails before the source patch and passes after it. I used ChatGPT as an AI assistant for this contribution to get an understanding of the problem and explore potential solutions. I worked on the code changes, ran the tests using Google Colab, and reviewed the implementation. I came across this project while looking for ML related open-source contributions. I am curious to learn about the RVFL/ELM networks and contribute to the project. I will look into handling the scalar "hidden_layer_sizes" parameter in "GFDLRegressor", add the relevant regression test, fix the lint issues, and take care of the rest of the feedback. Thanks for your time and guidance! |
|
Thanks for proposing the fix. Somewhere I suggested the same I am wondering if we would prefer that one-line fix or use the implementation in hidden_layer_sizes = self.hidden_layer_sizes
if not hasattr(hidden_layer_sizes, "__iter__"):
hidden_layer_sizes = [hidden_layer_sizes]
hidden_layer_sizes = list(hidden_layer_sizes)Pro: fewer lines of code; Con: In general, is there any reason |
That shouldn't happen if our array API testing is sufficient. If |
This PR fixes handling of scalar
hidden_layer_sizesvalues by converting them to one-dimensional NumPy arrays.fit()andpartial_fit()in both classifier implementations.pytest -q src/gfdl/tests/test_model.py: 5537 passed, 2 xfailed.Fixes #136