-
Notifications
You must be signed in to change notification settings - Fork 5
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
Mdn cleanup/deprecating hidden features #27
Mdn cleanup/deprecating hidden features #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this @anasbekheit !
It all looks good. I just made a couple of suggestions: keep hidden_net
and num_components
as required args
; use hidden_features
when it is passed and make sure it fits.
Once those are in, it's good to be merged.
Thanks again 🙏
Another comment: We are planning to re-license Please comment under this PR that you approve this re-licensing, otherwise we will not be able to merge the PR. Thanks! |
Co-authored-by: Jan <janfb@users.noreply.github.com>
Co-authored-by: Jan <janfb@users.noreply.github.com>
Co-authored-by: Jan <janfb@users.noreply.github.com>
Sure, no worries! I approve of the re-licensing of the code. |
Hi @janfb, thanks for taking the time to review, I committed your suggestions, let me know if anything else needs polishing. Cheers! |
Yes @anasbekheit , could you please merge |
I am not sure I understand, I did a git pull, there were no conflicts with main. |
My bad, only the rebasing would not work due to your changes. Squashing works just fine. |
Deprecated parameter
hidden_features
in mdn.pyProblem Description
Previously users were required to pass in
hidden_features
which is un-necessary and introduces the following problemshidden_net
output in their code in order to pass it properly which adds unnecessary overhead on the user.hidden_features
that doesn't matchhidden_net
output the code fails.Proposed Solution
Instead of requiring the user to provide
hidden_features
, the code should inferhidden_features
fromhidden_net
.Other Considerations
Code has to maintain backwards compatibility with code that is maybe using
hidden_features
as a positional argument.This is done by converting
hidden_features
,hidden_net
,num_components
into kwargs and manually checking for their existence.Misc
added
.idea/
to.gitignore
to support Idea IDEs