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

Add missing fields to Parameter #29

Closed
wants to merge 1 commit into from

Conversation

barucden
Copy link
Member

We were missing the fields nu and regularize_bias in the Parameter structure.

Fixes #28.

Discovered when working on JuliaML/LIBSVM.jl#98.

We were missing the fields `nu` and `regularize_bias` in the `Parameter`
structure.

Fixes JuliaML#28.

Discovered when working on JuliaML/LIBSVM.jl/#98.
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: The pull request successfully adds the missing fields 'nu' and 'regularize_bias' to the 'Parameter' structure, which were identified as necessary for the project. This enhancement is in line with the reported issue and the related work in the LIBSVM.jl repository.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Ensure that the addition of new fields is accompanied by updates to any associated documentation or examples where the 'Parameter' structure is used.
  • Consider whether any existing unit tests need to be updated or if new tests should be added to cover the changes made to the 'Parameter' structure.
  • Review the initialization and usage of the new fields across the codebase to maintain consistency and prevent any potential issues with default values or missing assignments.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@barucden barucden mentioned this pull request Jan 28, 2024
@ablaom
Copy link
Collaborator

ablaom commented Jan 28, 2024

Thanks for working on this @barucden . I'm not sure I have the knowledge to review this (and have not rights) but I noticed there appears to be some overlap with #26 (still open). In particular the two missing parameters are already added there. Is it possible that PR renders this one moot?

@barucden
Copy link
Member Author

True! I did not check the repository before diving into fixing the bug. #26 is definitely more complete, and we should go with that one. Nevertheless, that PR has been open for a few months already, which brings us to #27

@barucden barucden closed this Jan 28, 2024
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.

Crash due to misaligned pointer
2 participants