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

Wrapper interface #314

Merged
merged 6 commits into from
Jan 26, 2024
Merged

Wrapper interface #314

merged 6 commits into from
Jan 26, 2024

Conversation

ElGigi
Copy link
Contributor

@ElGigi ElGigi commented Jan 16, 2024

Added a Wrapper interface to identify enclosing models.

And thus allow for example to test if a included model implements a particular interface like LoggerAwareInterface.

@ElGigi ElGigi changed the title Wrapper interface and WrapperAware trait Wrapper interface Jan 19, 2024
@andrewdalpino
Copy link
Member

Cool! Thanks @ElGigi, I think we had something like this before but I can't remember why we got rid of it! I'll give it a closer look when I can.

Copy link
Member

@andrewdalpino andrewdalpino left a comment

Choose a reason for hiding this comment

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

Ok I left some comments ... I remember having an interface like this before, but I can't remember why we removed it exactly. My guess is that it didn't serve any internal use cases so we just dropped it.

I can think of a few external use cases for an interface like this. The one you mentioned is a good one. Do you have any in particular in your own projects?

src/Wrapper.php Outdated Show resolved Hide resolved
src/Wrapper.php Outdated Show resolved Hide resolved
@andrewdalpino
Copy link
Member

P.S. To fix the coding style automatically you can run compose fix from the project root

@ElGigi
Copy link
Contributor Author

ElGigi commented Jan 22, 2024

Ok I left some comments ... I remember having an interface like this before, but I can't remember why we removed it exactly. My guess is that it didn't serve any internal use cases so we just dropped it.

I can think of a few external use cases for an interface like this. The one you mentioned is a good one. Do you have any in particular in your own projects?

In our project, we abstract of the final model. To allow to define own model in sub-libraries.

The final service don't known the model but only interface, so to define if we set a logger or if the model is trainable, we need to retrieve the base estimator.

@ElGigi
Copy link
Contributor Author

ElGigi commented Jan 22, 2024

P.S. To fix the coding style automatically you can run compose fix from the project root

I do 👍

I didn't want to do that because it would modify too many files. Unrelated to my PR.

@andrewdalpino andrewdalpino changed the base branch from master to 2.5 January 26, 2024 02:28
@andrewdalpino andrewdalpino merged commit 4c16268 into RubixML:2.5 Jan 26, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants