tag-register all raw arrays #4116
Conversation
|
@Guruhegde thanks for the patch. i skid through the patch looks good... i've restarted the failed travis jobs, lets see the output and based on that we can merge it 👍 |
|
@Guruhegde as you can see there some formatting issues in the patch: you can use |
|
@vigsterkr sorry about the error. I will fix. |
| this->m_parameters->add(&m_num_classes, "m_num_classes", | ||
| "Number of classes."); | ||
|
|
||
| this->m_parameters->add_vector(&m_labels, &m_num_labels, "labels_array", |
There was a problem hiding this comment.
I think you don't need to change anything here.
| p->add_vector((float64_t**)param->m_parameter, type.m_length_y, | ||
| param->m_name); | ||
| watch_param( |
There was a problem hiding this comment.
(the class will be rewritten soon)
| m_parameters->add_vector((SGString<float64_t>**)&distance_matrix, &num_tasksqr, "distance_matrix", "distance between tasks"); | ||
| watch_param( | ||
| "distance_matrix", (SGString<float64_t>**)&distance_matrix, |
There was a problem hiding this comment.
I don't think this can work, these static casts don't make any sense
Could you remove you added watch_param statements here?
Then, in another PR, could you even comment out the add_vector calls?
There was a problem hiding this comment.
Questions:
Would you like me to comment out the add_matrix call having static_cast in parameter, i found one in ParameterCombination.cpp?
If so, shall i send single PR to address both(comment add_vector + add_matrix)?
While commenting the code, should i leave a remark with any issue no.?
I made necessary changes for #4110. will send PR soon.
Thanks.
|
Pls rebase against develop, the 4114 merge should not appear in here |
src/shogun/base/class_list.cpp.templ
Outdated
| @@ -75,7 +75,7 @@ void shogun::delete_object(CSGObject* object) | |||
| delete object; | |||
| } | |||
|
|
|||
| std::set<std::string> available_objects() | |||
| std::set<std::string> shogun::available_objects() | |||
There was a problem hiding this comment.
there are changes from another PR, you need to rebase.
Always check the diff !
|
Sorry, something else was merged that partially addressed these changes. You will need to rebase and resolve. Otherwise this is good to merge soon |
| this->m_parameters->add(&m_num_classes, "m_num_classes", | ||
| "Number of classes."); | ||
|
|
||
| this->m_parameters->add_vector(&m_labels, &m_num_labels, "labels_array", |
There was a problem hiding this comment.
I removed tag register here, by mistake i didn't remove new line. hope it's ok.
|
Addressed issues in the last commit. Tests pass. I hope this goes well. |
|
tada! :) |
|
Happy to see this! Thanks for the guidance :) Questions: While commenting the code, should i leave a remark with any issue no.? |
ghost
mentioned this pull request
|
Don’t worry about Parameter combination, we will take care of that |
ghost
deleted the
Made changes as suggested in #4101
All tests pass.