Skip to content

Add intRVFL to the models module #125

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

Merged
merged 13 commits into from
Feb 28, 2023
Merged

Add intRVFL to the models module #125

merged 13 commits into from
Feb 28, 2023

Conversation

denkle
Copy link
Collaborator

@denkle denkle commented Feb 26, 2023

Add intRVFL to the models module

Copy link
Member

@mikeheddes mikeheddes left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Denis! I refactored the code but since I'm not too familiar with intRVFL could you verify that I didn't break anything?

@mikeheddes mikeheddes linked an issue Feb 26, 2023 that may be closed by this pull request
@denkle
Copy link
Collaborator Author

denkle commented Feb 27, 2023

Thanks for the refactoring, Mike!
I like how the new code looks like.
Functionality-wise, it is all good, I only added a few minor edits.

Few minor edits made:

  1. train_loader in “UCI_benchmark_intRVFL.py” is not needed anymore with the current code. [Fixed].
  2. In “UCI_benchmark_intRVFL.py”, added transform() to data when calling “fit_ridge_regression()” - was affecting performance a lot.
  3. In “models.py”, kappa is not mentioned as a part of description in “Args”. [Fixed]
  4. In “UCI_benchmark_intRVFL.py”, ridge regression was missing (not a big effect on results though) when calling “fit_ridge_regression()”. [Fixed]

@denkle
Copy link
Collaborator Author

denkle commented Feb 27, 2023

Few additional considerations while going through the code:

  1. It could make sense to have alpha as an input to IntRVFLRidge() and then avoid calling it explicitly with “fit_ridge_regression()”.
  2. I would hide “one_hot_targets” under the hood in “fit_ridge_regression” as it is unclear what does it give to the user by explicitly seeing formation of one-hot encodings in “UCI_benchmark_intRVFL.py” but I see the extra hassle with sending in "num_classes".
  3. After refactoring, “fit_ridge_regression()” is a function within IntRVFLRidge() class. Before it was meant to be a separate function just because it is a primitive for a particular type of classifier (like centroids) so it does not have to stay exclusively within IntRVFLRidge(). But perhaps it is not so complicated so one could copy-paste this piece of code every time it is needed in some other place.
    All these considerations are not critical and perhaps not worth extra discussion so the final word about them is yours.

@mikeheddes
Copy link
Member

Thanks for spotting my bugs!

  • From what I could tell the alpha parameter is specific to the learning method ridge regression so if we end up adding more methods they could have their own parameters which are independent of the class instance. Let me know if I'm misinterpreting that?
  • The reason I moved the one-hot encoding outside ridge regression is because you had a comment that said something like you could also have multiple class vectors per class. By letting the user handle the one-hot encoding it could split one class over multiple class vectors. Do you think this makes sense?
  • I will refactor your last point

@denkle
Copy link
Collaborator Author

denkle commented Feb 28, 2023

  • I agree that the alpha is specific to the learning method ridge regression. I was probably assuming that if we have other methods for training the classifier we would create another class instance something like IntRVFLAbc() where "Abc" would refer to the specific classifier used for the model (currently IntRVFLRidge suggests that ridge regression is used as the classifier). Maybe it does not make sense to have multiple class instances and, instead, one should specify different ways of forming the classifier within the existing class so specifying "fit_abc()" method instead.
  • Okay, that is right. I have not made myself clear enough in that comment. It targeted prototype-based classifiers (like GLVQ) for ridge regression based classier, I am not sure if one could benefit from multiple class vectors per class. So for fit_ridge_regression(), I think we can safely have one-hot encodings inside the function, thus, having less details exposed to the user.
  • Great!

@mikeheddes mikeheddes merged commit 2effae2 into main Feb 28, 2023
@mikeheddes mikeheddes deleted the intRVFL_to_model branch February 28, 2023 18:48
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.

Add intRVFL to the models module
2 participants