Skip to content

forest-n-leaf is not updated by pruning!, so it reports the pre-pruning leaf count #15

Description

@masatoi

The forest-n-leaf slot is written only by make-forest and make-regression-forest. pruning! calls set-leaf-index-forest!, which updates each dtree-max-leaf-index, but nothing updates forest-n-leaf. The exported accessor therefore keeps reporting the pre-pruning leaf count.

Since forest-n-leaf is the natural accessor to reach for when measuring how much Global Pruning compacted the model — one of the features advertised in the README — this silently reports zero reduction.

Reproduction

SBCL 2.6.7, master @ b4e5ec5. Same setup as #14 (30 trees, 2000 synthetic data, pruning-rate 0.2):

(clrf:forest-n-leaf *forest*)  ; => 2759

;; ground truth
(reduce #'+ (mapcar #'cl-random-forest/src/random-forest::dtree-max-leaf-index
                    (clrf:forest-dtree-list *forest*)))  ; => 2759

(clrf:pruning! *forest* *learner* 0.2)

(clrf:forest-n-leaf *forest*)  ; => 2759   <- stale, unchanged

(reduce #'+ (mapcar #'cl-random-forest/src/random-forest::dtree-max-leaf-index
                    (clrf:forest-dtree-list *forest*)))  ; => 2542   <- actually pruned

Counting the leaves by traversal with do-leaf agrees with dtree-max-leaf-index (2542), so pruning itself is working correctly — only the cached slot is wrong.

Cause

make-forest (src/random-forest.lisp:660-662) is the only writer:

(setf (forest-n-leaf forest)
      (apply #'+ (mapcar #'dtree-max-leaf-index
                         (forest-dtree-list forest))))

pruning! (src/random-forest.lisp:1219-1226) ends with (set-leaf-index-forest! forest) and does not touch forest-n-leaf.

Impact

Cosmetic / reporting only, as far as I can tell. make-refine-learner (src/random-forest.lisp:816) and make-regression-refine-learner (src/random-forest.lisp:1053) both compute input-dim by summing dtree-max-leaf-index directly rather than reading forest-n-leaf, so the refine pipeline is unaffected. grep shows forest-n-leaf has no other readers inside src/.

Suggested fix

Move the sum into set-leaf-index-forest! so every caller (both make-forest and pruning!) keeps it consistent:

(defun set-leaf-index-forest! (forest)
  (dolist (dtree (forest-dtree-list forest))
    (set-leaf-index! dtree))
  (let ((sum 0)
        (offset (forest-index-offset forest)))
    (setf (aref offset 0) 0)
    (loop for dtree in (forest-dtree-list forest)
          for i from 1 below (forest-n-tree forest)
          do (setf sum (+ sum (dtree-max-leaf-index dtree)))
             (setf (aref offset i) sum)))
  (setf (forest-n-leaf forest)
        (reduce #'+ (mapcar #'dtree-max-leaf-index (forest-dtree-list forest)))))

and drop the now-redundant setf in make-forest / make-regression-forest.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions