Feature/sbachmei/refactor observation registrations and other minor updates #437
Conversation
stevebachmeier
requested review from
albrja,
collijk,
hussain-jafari,
patricktnast and
rmudambi
as code owners
stevebachmeier
changed the base branch from
main
to
epic/MIC-4968-results-processing
| when=when, | ||
| included_columns=["event_time"] + requires_columns + requires_values, | ||
| results_formatter=results_formatter, | ||
| if observation_type == ConcatenatingObservation: |
There was a problem hiding this comment.
Is this the best way to do this? Alternatively I can add included_columns as a BaseObserver attribute that just defaults to an empty list everywhere
There was a problem hiding this comment.
How about in results_interface.register_concatenating_observation()?
…vivarium into feature/sbachmei/refactor-observation-registrations-and-other-minor-updates
| [pd.DataFrame, pd.DataFrame], pd.DataFrame | ||
| ] = _raise_missing_stratified_observation_results_updater, | ||
| results_updater: Callable[[pd.DataFrame, pd.DataFrame], pd.DataFrame] = partial( | ||
| StratifiedObservation._raise_missing, "results_updater" |
There was a problem hiding this comment.
Upon reflection, isn't this error going to be thrown when the updater is called? Don't we want it to be thrown during registration instead? We could throw on registration if we define a default method (e.g. BaseObserver.foo) and then have this:
def register_stratified_observation(...):
if results_updater == BaseObserver.foo:
raise ValueError(...)| self._add_resources(requires_values, SourceType.VALUE) | ||
| observation_type, | ||
| is_stratified: bool, | ||
| **kwargs, |
There was a problem hiding this comment.
Since name is required, should we include it as an explicit argument here? Same goes for requires_columns/values, which enable us not to need to del them.
There was a problem hiding this comment.
Good idea. I'll include pop_filter and when as well (not precisely required b/c of defaults but is common among everything)
| when=when, | ||
| included_columns=["event_time"] + requires_columns + requires_values, | ||
| results_formatter=results_formatter, | ||
| if observation_type == ConcatenatingObservation: |
There was a problem hiding this comment.
How about in results_interface.register_concatenating_observation()?
| @@ -57,6 +64,10 @@ def __init__( | |||
| results_formatter=results_formatter, | |||
| ) | |||
|
|
|||
| @property | |||
| def stratifications(self) -> None: | |||
There was a problem hiding this comment.
Shouldn't this be defined on BaseObservation?
There was a problem hiding this comment.
Ah, good catch - this is lingering from when I was trying to make @DataClass work for everything
stevebachmeier
merged commit d8a97f8
into
epic/MIC-4968-results-processing
stevebachmeier
deleted the
feature/sbachmei/refactor-observation-registrations-and-other-minor-updates
branch
Refactor observation registrations and other minor updates
Description
Changes and notes
This PR is a sort of follow-on to the previous one because that one wasalready getting a bit bloated. The primary change here is that it consolidates
the context and manager observation registration functions into a single
one. Note that we have kept explicit
register_<type>_observationfunctionsexposed in interface.
There are a few other minor updates as well stemming from other comments
on the previous PR.
Most unique changes are contained in specific commits.
Testing
tests pass