[ML] Instrumentation information for boosted tree learning #1031
Conversation
There was a problem hiding this comment.
I've done a first pass through. Overall looks great (and good job on reducing test duplication). I've made some minor comments. My most significant comment is I'd like to avoid the double inheritance: we have historically mandated against this. I can see why you've done it so will ponder the design a bit to see if I can make an alternative suggestion.
lib/maths/CBoostedTreeImpl.cc
Outdated
| lossMoments.add(loss); | ||
| m_FoldRoundTestLosses[fold][m_CurrentRound] = loss; | ||
| numberTrees.push_back(static_cast<double>(forest.size())); | ||
| m_Instrumentation->lossValues(std::to_string(fold), std::move(lossValues)); |
There was a problem hiding this comment.
Is there any reason to convert this to a string here? I can't think of one.
There was a problem hiding this comment.
Yes, when I am going to write the loss values in rapid json, fold becomes the key and it has to be a string. I cannot convert it to string during writing since the string has to exist in memory until the writer finishes writing, otherwise, it just puts some random bytes as the key.
|
Thank you for your comments @tveasey. I addressed your comments. It would be great if you could review the changes and let me know if it's ok. |
There was a problem hiding this comment.
Looks good @valeriy42. A couple more minor suggestions, but otherwise looks good. I don't think this needs any further review so I'll approve.
|
retest |
|
retest |
) This PR enables instrumentation of the supvervised learning jobs. I needed to add maths::CDataFrameTrainBoostedTreeInstrumentationInterface to have an interface for storing validation error results, hyperparameters, and timing. Since I am passing iteration directly over the new interface, I also refactored nextStep to use an optional string phase as an argument. At the moment, we are not tagging phases, but we intend to in the future. Thus, I added a TODO there. Finally, I extracted the common routines for generating regression and classification data into CDataFrameAnalyzerTrainingFactory. It is not used for feature importance testing so far, cause the generation procedure there is a little bit different. I intend to unify the code in a follow-up PR.
This PR enables instrumentation of the supvervised learning jobs. I needed to add maths::CDataFrameTrainBoostedTreeInstrumentationInterface to have an interface for storing validation error results, hyperparameters, and timing. Since I am passing iteration directly over the new interface, I also refactored nextStep to use an optional string phase as an argument. At the moment, we are not tagging phases, but we intend to in the future. Thus, I added a TODO there. Finally, I extracted the common routines for generating regression and classification data into CDataFrameAnalyzerTrainingFactory. It is not used for feature importance testing so far, cause the generation procedure there is a little bit different. I intend to unify the code in a follow-up PR. Backport for #1031.
This PR enables instrumentation of the supvervised learning jobs. I needed to add
maths::CDataFrameTrainBoostedTreeInstrumentationInterfaceto have an interface for storing validation error results, hyperparameters, and timing.Since I am passing
iterationdirectly over the new interface, I also refactorednextStepto use an optional stringphaseas an argument. At the moment, we are not tagging phases, but we intend to in the future. Thus, I added a TODO there.Finally, I extracted the common routines for generating regression and classification data into
CDataFrameAnalyzerTrainingFactory. It is not used for feature importance testing so far, cause the generation procedure there is a little bit different. I intend to unify the code in a follow-up PR.