I think this PR includes by mistake unrelated Cython files from the sklearn/metrics folder.

Cross-linking: #20648

Hi @jjerphan @ogrisel and @glemaitre just wanted to do a light ping to see if there's any feedback regarding this and if this is good to merge? This is probably the most simple change I proposed in the roadmap for improving trees.

@jshinm has volunteered to try to address the comments you outlined in the code. Thanks!

I re-ran the asv benchmark after closing all open apps on my Mac M1:

(sklearn) adam2392@Adams-MacBook-Pro asv_benchmarks % asv compare --split upstream/main crit

Benchmarks that have improved:

       before           after         ratio
     [3e47fa91]       [106b01cf]
     <main>           <crit>    
-            555M             495M     0.89  ensemble.RandomForestClassifierBenchmark.peakmem_fit('sparse', 1)

Benchmarks that have stayed the same:

       before           after         ratio
     [3e47fa91]       [106b01cf]
     <main>           <crit>    
             267M             281M     1.05  ensemble.RandomForestClassifierBenchmark.peakmem_fit('dense', 1)
             218M             218M     1.00  ensemble.RandomForestClassifierBenchmark.peakmem_predict('dense', 1)
             439M             435M     0.99  ensemble.RandomForestClassifierBenchmark.peakmem_predict('sparse', 1)
          4.82±0s          4.75±0s     0.98  ensemble.RandomForestClassifierBenchmark.time_fit('dense', 1)
       7.25±0.01s          7.13±0s     0.98  ensemble.RandomForestClassifierBenchmark.time_fit('sparse', 1)
          137±8ms        132±0.4ms     0.97  ensemble.RandomForestClassifierBenchmark.time_predict('dense', 1)
         980±40ms         907±50ms     0.93  ensemble.RandomForestClassifierBenchmark.time_predict('sparse', 1)
  0.7502017055240395  0.7502017055240395     1.00  ensemble.RandomForestClassifierBenchmark.track_test_score('dense', 1)
  0.8656423941766682  0.8656423941766682     1.00  ensemble.RandomForestClassifierBenchmark.track_test_score('sparse', 1)
  0.9971484595841501  0.9971484595841501     1.00  ensemble.RandomForestClassifierBenchmark.track_train_score('dense', 1)
  0.9996123288718864  0.9996123288718864     1.00  ensemble.RandomForestClassifierBenchmark.track_train_score('sparse', 1)

There are no performance regressions with including this refactoring. However, not sure why we saw an improvement in memory for sparse data. Might be chance? Saw similar results when I ran it yesterday tho (#24678 (comment)) cc: @jjerphan

Our next steps are for @jshinm to re-run the asv benchmarks and to also address the code changes.

Just wanted to follow up to get some idea what might be blocking this?

We have several downstream PRs that fully flush out the modularity of criterion/splitter in the right directions w/o introducing performance degradation, so don't wanna continue working on those if something is drastically going to change here.

Thanks!

Just wanted to follow up to get some idea what might be blocking this?

It has not got response nor attention mainly because of maintainers' lack of time, unfortunately.

@adam2392: could you change the target branch of this PR to refactor/cython-tree-class-hierarchy?

Done!

The only CI issue is the CHANGELOG, but IIRC, that is fine.

Here is an example of how such a refactoring is utilized: https://github.com/neurodata/scikit-tree/blob/main/sktree/tree/_unsup_criterion.pyx

Good job!

What happened here? Why was it closed?

Sorry for the annoyance.

@adrinjalali mentioned the scikit-learn:refactor/cython-tree-class-hierarchy branch to me which seemed not to be used. I did not remember that this PR target to it, hence I deleted it, closing this PR. 🤦

I just have repushed the branch since I had a copy on my clone locally.

@adam2392: I cannot reopen this PR, can you?

Sure I can re-open this PR. May I ask what is the desire in doing so? Do we think this might move forward? I don't want to get my hopes up :p

Do I target the now deleted branch, or do I target main?

Do we think this might move forward?

I can't guarantee this will move forward. We need to have people be involved on the Cython side of the project (I personally am contributing to scikit-learn on my free time solely).

refactor/cython-tree-class-hierarchy is available again. Can you target it if you want to continue this PR?