Skip to content

Add residual estimator and erase previous one - #10

Open
SebastienMelo wants to merge 1 commit into
aperezlebel:mainfrom
SebastienMelo:residual_estimator
Open

Add residual estimator and erase previous one#10
SebastienMelo wants to merge 1 commit into
aperezlebel:mainfrom
SebastienMelo:residual_estimator

Conversation

@SebastienMelo

Copy link
Copy Markdown

The PR adds the new estimator for the grouping loss as well as the epistemic risk estimator.

Comment thread glest/core.py

return self

def GL(self, psr: str = "brier"):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The psr argument is not used and can be removed. Same across methods of GLEstimator.

Comment thread glest/core.py
classifier,
partitioner: str | Partitioner = "decision_tree",
train_size: float = 0.5,
estimator: str = "hgb",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This argument is not used

Comment thread glest/core.py
def from_name(
cls,
name: str,
verbose: int = 0,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This argument is not used

Comment thread glest/core.py
strategy: str = "uniform",
binwise_fit: bool = True,
raise_on_fit_error: bool = False,
verbose: int = 0,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This argument is not used anymore.

Comment thread glest/core.py
self.partitioner = partitioner
self.cv = cv
residual_estimator: ResidualEstimator = None,
# t: float = 0.5,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove commented code

Comment thread glest/core.py
if residual_estimator is not None
else None
)
# self.t = t

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same

@aperezlebel aperezlebel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for improving the estimator! Could you update the tests with the new estimator? As it stands they are still using the previous one and are thus failing.

Comment thread glest/plot.py
figsize=(3, 3),
figsize=(4, 4),
)
_scatter_kw = dict(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not used

Comment thread glest/plot.py
Comment on lines 64 to 80
@@ -73,85 +74,111 @@ def grouping_diagram(
)
_legend_kw = dict(
framealpha=0,
loc='lower center',
loc="lower center",
bbox_to_anchor=(0.5, 1.1) if plot_hist else (0.5, 1),
ncols=2,
)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same

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