Update deprecated functions in RF and GBT Examples #2878
Conversation
|
/intelci: run |
|
/intelci: run |
md-shafiul-alam
requested review from
Alexsandruss,
samir-nasibli and
Alexandr-Solovev
as code owners
|
I am not sure what the plan for removal of the deprecated interfaces is but I would leave these in the repo for now. Would it be possible just to update the examples so that they are using the existing non-deprecated replacement interfaces? |
It was an interesting decision to deprecate functions which would have been just fine to add the new parameters with default values. The deprecated functions do exactly that. It doesn't seem to me that the deprecation was necessary. For the matter of removing the deprecated functions, there is actually no functional difference. If anyone calls with previous definitions, they will still work. However, I am open to go either way. |
|
@Alexsandruss thoughts? |
|
|
/intelci: run |
| DAAL_DEPRECATED NodeId addLeafNode(const TreeId treeId, const NodeId parentId, const size_t position, const size_t classLabel) | ||
| { | ||
| return addLeafNode(treeId, parentId, position, classLabel, 0); | ||
| } |
There was a problem hiding this comment.
Major binary version of library should be increased since ABI would change and I'm unsure if it's feasible for next major release as some additional work related to versioned shared libraries is needed.
I'm OK with other non-deletion changes, but you should verify binary backward compatibility on both of Linux and Windows.
There was a problem hiding this comment.
Do you suggest not deleting for now?
There was a problem hiding this comment.
Yes, postpone PR merge until next major release and then bump major binary version.
md-shafiul-alam
changed the title
|
@Alexsandruss Ignored any API changes for this PR. Would you please review again? |
|
/intelci: run |
|
Should we remove deprecated functions in the same time as update examples? |
|
and we increased major binary version, so I guess now it makes sense to return function deletions |
md-shafiul-alam
force-pushed
the
remove-deprecated-examples
branch
from
6e55c27
to
af3c25e
Compare
I think yes - this generally looks good to me but I don't think optional args should be added. We have opportunity to update the API and this usage is already deprecated. |
cpp/daal/include/algorithms/decision_forest/decision_forest_classification_model_builder.h
Outdated
Show resolved
Hide resolved
|
/intelci: run |
|
Private CI is supposed to fail at build step because of API break. Here is the job with updated infra branch: https://intel-ci.intel.com/ef6b44cb-fdf0-f1fb-b313-a4bf010d0e2e |
|
/intelci: run |
|
The private CI timeout and warning are unrelated to the PR. |
Description
There are a few functions in Random Forest and Gradient Boosted tree in DAAL that have been deprecated and replaced by newer definitions which expects one or two parameters more. Because of the deprecation flags CI throws warnings on windows. The deprecated functions are being used from examples and some tests on ci. This PR modifies the examples to use newer definitions. Public CI failure will be addressed on #2891