For the node-level boostrapping we want to use the same helper functions as for tree-level boostrapping (_get_n_samples_bootstrap and _generate_sample_indices). We have copy-pasted the code for now because of cycle dependency issues (these functions should be in utils for example).

We do not understand why the implementation we propose do not pass the tests, since we try not to change the behavior of the code. Even when we set boot_size to X.shape[0], the tests do not pass. Do you have any lead ?

The initial failures here are in lining, not test failure:

sklearn/tree/_classes.py:52:1: E303 too many blank lines (3)
def _get_n_samples_bootstrap(n_samples, max_samples):
^
sklearn/tree/_classes.py:418:26: W291 trailing whitespace
        proportion = 0.99 
                         ^
sklearn/tree/_classes.py:420:80: E501 line too long (80 > 79 characters)
        ind = _generate_sample_indices(self.random_state, X.shape[0], boot_size)
                                                                               ^
sklearn/tree/_classes.py:422:80: E501 line too long (87 > 79 characters)
            builder.build(self.tree_, X[ind], y[ind], sample_weight, X_idx_sorted[ind])
                                                                               ^
sklearn/tree/_classes.py:424:80: E501 line too long (82 > 79 characters)
            builder.build(self.tree_, X[ind], y[ind], sample_weight, X_idx_sorted)

But yes, other tests failing seem to imply that your code is producing different models, and deserve investigating. Maybe choose one of those falling tests, and run them while adding debug output to guide you to the point of failure

@jnothman Thanks for your answer about stratification. Indeed, the current implementation always bootstraps the data at each node of the trees, without any stratification, as the bootstrapping at the tree level. Should we stratify the bootstrap at the node level ? Should we also perform stratified bootstrapping at the tree level ?

@cmarmo @thomasjpfan
Are you able to assist with this PR?

Hi @reshamas, @JosephTLucas, I can't reproduce the test failure, as it happens for linux 32bit systems. I'm not able to comment about the algorithm, but FYI, if the failure is really a problem of computing precision on different architectures, there is a way to skip tests on 32bit system adding

@skip_if_32bit

marker just before the faulting test.

I have added @skip_if_32bit to the test, and now the CI is ok. Thanks @cmarmo.

I'm not sure if I follow entirely. Right now you're doing tree-level boostrapping, right? To do node-level bootstrapping, you'd have to change the Cython code, I assume?
I feel like it might be worth to do tree-level bootstrapping with less samples first as an option, using bootstrap=float as @jnothman suggested, I think node-level bootstrapping will be much harder.

btw, I think #13227 might be somewhat related.

@amueller We intend to do bootstrapping at the node level. I think that bootstrapping at the tree level has already been implemented and merged (#14682). The argument bootstrap is still a boolean, and the sampling rate is provided by the argument max_samples.
We thought we could implement bootstrap at the node level without changing the Cython code.

@ab-anssi oh thanks, I didn't remember #14682.
Can you say how you would do the node-level bootstrapping without changing the Cython code? I don't really see how.

Our current implementation modifies the fit function of the BaseDecisionTree class. But, now that I look at it again, it does not seem to be the right place (there is not recursive call here). I am going to think about a better (right !) way to implement this feature.

@JosephTLucas @ab-anssi
Are you still working on this PR?

@JosephTLucas @ab-anssi
Are you still working on this PR?

No, closed it.