Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Feature/refactor value #558

Draft
wants to merge 279 commits into
base: develop
Choose a base branch
from
Draft

WIP: Feature/refactor value #558

wants to merge 279 commits into from

Conversation

mdbenito
Copy link
Collaborator

@mdbenito mdbenito commented Apr 9, 2024

Description

This PR closes #XXX

closes #539
closes #187

Changes

Checklist

  • Wrote Unit tests (if necessary)
  • Updated Documentation (if necessary)
  • Updated Changelog
  • If notebooks were added/changed, added boilerplate cells are tagged with "tags": ["hide"] or "tags": ["hide-input"]

…tion.py - rename source-file to git-split-temp
…tion.py - resolve conflict and keep both files
@@ -92,13 +92,13 @@ class DummyModel(SupervisedModel):
def __init__(self) -> None:
pass

def fit(self, x: NDArray, y: NDArray) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this module moved to the new valuation package?

P.S. This comment is not about this specific line. I just found it easier to comment on it.

)
ensure_backend_has_generator_return()

self.utility.training_data = data
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.utility.training_data = data
self.utility.with_dataset(data)

"""
...

def fit(self, data: Dataset):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not change the signature to this to allow using the dataset set on the utility, if there is one of course?

Suggested change
def fit(self, data: Dataset):
def fit(self, data: Dataset | None = None):

Co-authored-by: Kristof Schröder <[email protected]>
def __init__(
self,
model: ModelT,
scorer: Scorer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The scorer is no longer the model's default scorer. Was it changed on purpose?

self,
utility: UtilityBase,
sampler: IndexSampler,
is_done: StoppingCriterion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comes out of a discussion with @schroedk: is_done sounds like a bool. We should discuss renaming this across all methods to something like stopping or stop_criterion

@janosg janosg mentioned this pull request Jul 9, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants