Sbachmei/mic 6380/convert pipelines to attribute pipelines #647
Conversation
…les to use attribute pipelines; type hint updates throughout
stevebachmeier
requested review from
albrja,
hussain-jafari,
patricktnast and
rmudambi
as code owners
| @@ -68,7 +70,7 @@ def apply_force(self, index: pd.Index[int], acceleration: pd.DataFrame) -> pd.Da | |||
| def calculate_force(self, neighbors: pd.DataFrame) -> pd.DataFrame: | |||
| pass | |||
|
|
|||
| def _get_pairs(self, neighbors: pd.Series[int], pop: pd.DataFrame) -> pd.DataFrame: | |||
| def _get_pairs(self, neighbors: pd.Series[Any], pop: pd.DataFrame) -> pd.DataFrame: | |||
There was a problem hiding this comment.
Was this incorrectly typed before?
There was a problem hiding this comment.
Yeah, something is evidently making it typed as a float somewhere along the way so I just changed it to Any out of laziness. I'll switch it to int | float at least. This is just tricky to deal w/ b/c we don't have any tests for boids.
| f"{self.rate_name}.population_attributable_fraction", | ||
| source=lambda index: [pd.Series(0.0, index=index)], | ||
| preferred_combiner=list_combiner, | ||
| preferred_post_processor=union_post_processor, | ||
| ) | ||
| self.transition_rate = builder.value.register_rate_producer( | ||
| self.transition_rate = builder.value.register_attribute_rate_producer( |
There was a problem hiding this comment.
Not: I think the old name is fine here. It didn't have "value" in it.
There was a problem hiding this comment.
it's a calling a totally different method
…meuw/vivarium into sbachmei/mic-6380/convert-pipelines-to-attribute-pipelines
…an AttributePipeline by definition
| @@ -512,7 +512,7 @@ def test_move_simulants_to_end(base_config: LayeredConfigTree) -> None: | |||
| assert step_modifier_component.ts_pipeline_value.index.equals(full_pop_index) | |||
| assert np.all( | |||
| sim._clock.simulant_next_event_times(evens) | |||
| == sim._clock.stop_time + sim._clock.minimum_step_size | |||
| == sim._clock.stop_time + sim._clock.minimum_step_size # type: ignore [operator] | |||
There was a problem hiding this comment.
I just don't want to think about this here - not really sure the best way to handle the different types in stop time vs step_size. Happy to look closer if people want though
| "test_dataframe_attribute", source=dataframe_attribute_source | ||
| ) | ||
| bad_pipeline = manager.register_attribute_producer( | ||
| "test_bad_attribute", source=bad_attribute_source # type: ignore [arg-type] |
There was a problem hiding this comment.
Ignoring b/c this is precisely what we're testing here via pytest.raises
| @@ -676,3 +736,8 @@ def get_value(self, name: str) -> Pipeline: | |||
| """ | |||
| return self._manager.get_value(name) | |||
|
|
|||
| # TODO: [MIC-5452] Remove this method (attributes should be obtained via population views) | |||
| def get_attribute(self, name: str) -> AttributePipeline: | |||
There was a problem hiding this comment.
NOTE: this will NOT stick around for the long haul (the manager method might, just not this interface method). The way we intend for users to get attributes is via the population_view.get() method. This is just temporarily needed in a few loations (pytests and examples) until that interface gets updated.
Convert Pipelines to AttributePipelines
Description
Changes and notes
This does a few things:
register_attribute_modifierinterface method I forgot in the previous PRregister_attribute_rate_producermethod. I wasn't going to do this at firstbut realized I couldn't easily reuse the
register_rate_producersince that immediatelycalls
get_value()whereas we need to be callingget_attribute().NOTE: I did NOT convert the one and only vivarium-level pipeline, "simulant_step_size".
I think some more discussion is needed about exactly what AttributePipelines (and
their sources and modifiers) need to return. The only thing potentially keeping this from
just being an AttributePipeline is that the source isn't returning a pd.Series, but rather a
list of pd.Series! But since the post-processor is a list combiner, the callable does
indeed return a pd.Series. It should be easy to convert if we decide to. In fact, I tried it
and only one test failed (because the mocking was no longer being done correctly).
Testing
Tests pass.
NOTE: I did NOT run the boids example to make sure it works. I'm not really even sure how.