Skip to content

implemented multiclass for svc #308

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 4 commits into from
Jun 16, 2025

Conversation

DanielLacina
Copy link
Contributor

No description provided.

@DanielLacina DanielLacina requested a review from Mec-iS as a code owner June 15, 2025 15:06
@DanielLacina
Copy link
Contributor Author

I messed up. My .git directory got corrupted and then I deleted it. Then I initialized a new .git folder and forced push the code but that erased the commit history. Im sorry about that.

@DanielLacina
Copy link
Contributor Author

DanielLacina commented Jun 15, 2025

impl<'a, TX: Number + RealNumber, TY: Number + Ord, X: Array2<TX>, Y: Array1<TY>>
    SupervisedEstimatorBorrow<'a, X, Y, SVCParameters<TX, TY, X, Y>> for SVC<'a, TX, TY, X, Y>
{
    fn new() -> Self {
        Self {
            classes: Option::None,
            instances: Option::None,
            parameters: Option::None,
            w: Option::None,
            b: Option::None,
            phantomdata: PhantomData,
        }
    }
    fn fit(
        x: &'a X,
        y: &'a Y,
        parameters: &'a SVCParameters<TX, TY, X, Y>,
    ) -> Result<Self, Failed> {
        SVC::fit(x, y, parameters)
    }
}```

@DanielLacina
Copy link
Contributor Author

Isnt this the API?

@DanielLacina
Copy link
Contributor Author

Anyways Ill fix it.

@DanielLacina
Copy link
Contributor Author

PR is ready for review.

@Mec-iS
Copy link
Collaborator

Mec-iS commented Jun 15, 2025

Good! all checks passed.

ok, this is better but you didn't need to open a new PR, see also -> #306 (comment)

This PR's code is closer to what I had in mind just: when you add a method to an existing interface, like multiclass_fit, it goes at the end of the list (not at the beginning) to not break the reading convention. Same for the tests, you don't add the top but at the bottom. I know this sounds like nitpicking but it is good code hygiene.

@DanielLacina
Copy link
Contributor Author

alright Ill change it accordingly

@DanielLacina
Copy link
Contributor Author

@Mec-iS fixed

@Mec-iS
Copy link
Collaborator

Mec-iS commented Jun 16, 2025

Good one 🚀 I will merge it when the checks have finished. It will be released with v0.4.2

Copy link
Collaborator

@Mec-iS Mec-iS left a comment

Choose a reason for hiding this comment

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

Thanks!

@Mec-iS Mec-iS merged commit 730c0d6 into smartcorelib:development Jun 16, 2025
11 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