Register parameters in tags (#4117) #4123
Conversation
ghost
mentioned this pull request
src/shogun/classifier/svm/SVMSGD.cpp
Outdated
| watch_param("count", &count); | ||
|
|
||
| m_parameters->add(&use_bias, "use_bias", "Indicates if bias is used."); | ||
| watch_param("use_bias", &use_bias); |
There was a problem hiding this comment.
Actually, we just realised that we probably want to keep the comment as well.
But that requires a minor change to the registration method .... Let me cook something up and ping you
|
@lisitsyn Would you mind adding the possibility to add a comment in the parameters? |
|
@karlnapf yes, will add that. |
|
In #4132 I am adding parameter description. Lets rebase once it is merged and add these descriptions here as well. Otherwise the descriptions would get lost one day. |
|
Sure. I will add descriptions in this PR. |
|
@Guruhegde I just merged #4132 so you can rebase and add descriptions. |
|
I added the description of scalar parameters. I will send another PR to add the description for vector and matrix type. |
|
Remark: I ran tag parameter coverage test. Equality check with old parameter and tag parameter fails even though both lists have all & same parameters, this is because of the different order of parameter names in two lists. The solution I tried, sort old parameter names, which works. |
|
@Guruhegde yes we need to sort. It used to be a set, but was recently changed to a vector....thats the danger of using the |
| watch_param( | ||
| "pos_pseudo", &m_pos_pseudo, | ||
| AnyParameterProperties( | ||
| "pseudo count for positive class", MS_NOT_AVAILABLE, |
There was a problem hiding this comment.
@Guruhegde I have one more request to keep this cleaner:
Can you make the second two parameter of the AnyParameterProperties constructor have default values set to false.
And then don't put MS_NOT_AVAILABLE every time, but only the descriptions ...
There was a problem hiding this comment.
This way, things will look way more neat
There was a problem hiding this comment.
There was a problem hiding this comment.
Do a separate PR since that is very small and minimal. Then rebase this one here
ghost
mentioned this pull request
|
@karlnapf Hi this is ready for review:) |
|
Thanks! |
|
I think this could be re-ignited now :) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@karlnapf isn't this what we already do? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I think this is probably still useful? Rebase and merge? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue is now being closed due to a lack of activity. Feel free to reopen it. |
According to SGObjectAll test case, registered parameters in tags to match old parameters for all classes.
Exceptions:
For class LatentSOSVM & SVMOcas, old parameters and tag parameters don't match yet.(I am not able to find those classes, please help me find those).
@karlnapf There are many classes in this PR where static_cast on parameters preset. you asked in PR #4116 not add watch_param call & to comment add tags call in separate PR.
Would you like me to do this all together in separate PR? (I am curious what is the exact reason for that suggestion)