Sc/hier shrinkage #139
Sc/hier shrinkage #139
Conversation
…d predictionweights, R displays these tree info contains two new vectors for this reconstruct tree still does not have these features plotting uses certain logic to e.g. determine leaf node which might need to be changed
…length, corrected earlier test as well
…llow it to be passed in as a parameter, currently only default values are specified
… store split features and split node sizes separately
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
… working, additional information vectors created are now exposed in the python package - average_count and split_count
…fied to only expose one paraeter to the user not two, predictWeightsFull, valuesFull etc are now renamed post refactor
Python/random_forestry/forestry.py
Outdated
| @@ -643,6 +643,7 @@ def _aggregation_oob( | |||
| exact: bool, | |||
| return_weight_matrix: bool, | |||
| training_idx: Optional[np.ndarray], | |||
| hierShrinkageLambda: float, | |||
There was a problem hiding this comment.
nit: Can we comply with the snake case conventions in Python?
| if hierShrinkageLambda is not None: | ||
| if hierShrinkageLambda < 0: | ||
| raise ValueError("Hierarchical shrinkage parameter must be positive!") | ||
| else: |
There was a problem hiding this comment.
This else can be removed, same with the parallel hierShrinkageLambda checks below, to avoid unnecessary nesting.
There was a problem hiding this comment.
Have just pushed a change - is that what you had in mind @edwardwliu
There was a problem hiding this comment.
Made another loop, think we can merge in after this round, pending if @theo-s has comments.
Python/extension/api.h
Outdated
| @@ -110,7 +110,9 @@ extern "C" { | |||
| bool verbose, | |||
| std::vector<double>& predictions, | |||
| std::vector<double>& weight_matrix, | |||
| std::vector<size_t> training_idx | |||
| std::vector<size_t> training_idx, | |||
| bool hierShrinkage, | |||
There was a problem hiding this comment.
nit: if possible, let's switch to snake case here as well and for new usages throughout the Python API
R/R/forestry.R
Outdated
| # hierarchical shrinkage factor must be positive | ||
| if (hierShrinkageLambda < 0){ | ||
| stop("The value of the hierarchical shrinkage parameter must be positive") | ||
| } else { |
There was a problem hiding this comment.
Same unnecessary else statement and below as well in this R file.
There was a problem hiding this comment.
Have just pushed a change - is that what you had in mind
No description provided.