Skip to content

Conversation

dengdifan
Copy link
Contributor

Replace pyrfr with sklearn's RF

Copy link
Collaborator

@benjamc benjamc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just some docstrings and questions :)


@staticmethod
def get_intensifier( # type: ignore
scenario: Scenario,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the intensifier and initial design are just copy-pasted from the MF facade right? Nothing has changed here?

n_instances: int,
lock: threading.Lock,
) -> None:
"""Collect predictions from a single estimator. However, we sum the results from all instances"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add arg descriptions

Validate X whenever one tries to predict, apply, predict_proba.
It is based on rf regressor from sklearn 1.6.1:
https://github.com/scikit-learn/scikit-learn/blob/99bf3d8e4eed5ba5db19a1869482a238b6223ffd/sklearn/ensemble/_forest.py#L629
However, we add another parameter to allow the model to ignore feature checking (this is done after
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would we want to ignore feature checking? add to docstring

cross_trees_variance: bool = False,
criterion: str = "squared_error",
splitter: str = "random", # Should not be changed
max_depth: int = 2**20, # TODO: HP: None is default in sklearn, here it's 2**20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewrite comments, afaik there is no TODO. these were the HPs of the previous RF, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in all cases, keep the information somewhere

Copy link
Collaborator

@benjamc benjamc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some comments left to address

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants