Maintain lookup table value column order #521
Conversation
| @property | ||
| def standard_lookup_tables(self) -> List[str]: |
There was a problem hiding this comment.
it didn't seem to be using this?
There was a problem hiding this comment.
I don't remember what this was for but Rajan and I had tried to add some feature like this but I think this is deprecated now.
stevebachmeier
requested review from
albrja,
collijk,
hussain-jafari,
patricktnast and
rmudambi
as code owners
stevebachmeier
force-pushed
the
sbachmei/mic-5205/bugfix-lookup-table-non-deterministic-list
branch
from
e8d49c1 to
0f74c92
Compare
| @@ -610,31 +610,38 @@ def build_lookup_table( | |||
| if isinstance(data, list): | |||
| return builder.lookup.build_table(data, value_columns=list(value_columns)) | |||
| if isinstance(data, pd.DataFrame): | |||
| all_columns = set(data.columns) | |||
| duplicated_columns = set(data.columns[data.columns.duplicated()]) | |||
| if duplicated_columns: | |||
src/vivarium/component.py
Outdated
| col | ||
| for col in all_columns | ||
| if col not in value_columns and col not in bin_edge_columns | ||
| ] |
There was a problem hiding this comment.
Aren't lists not ordered? So if weh ave a different seed they could be in a different order?
There was a problem hiding this comment.
I'd also say make all this logic for getting the different columns a helper function/
There was a problem hiding this comment.
lists are ordered. Since all_columns is a list (ordered) of the data.columns (ordered), and we're building this up using list comprehension (col for col in all_columns...), it will build up the list in the same order as all_columns
| key_columns: Union[List[str], Tuple[str, ...]], | ||
| parameter_columns: Union[List[str], Tuple[str, ...]], | ||
| value_columns: Union[List[str], Tuple[str, ...]], | ||
| key_columns: Sequence[str], |
There was a problem hiding this comment.
I'm confused how this is a Sequence if you originally defined it as [] but I haven't really used Sequence.
There was a problem hiding this comment.
ultimately it can be any sequence (i.e. list or tuple). But it's kinda weird b/c as you said we instantiate it specifically as a list. But we were already saying Union[list, tuple] anyway! So I dunno - I think we're just a little inconsistent here in general
| @property | ||
| def standard_lookup_tables(self) -> List[str]: |
There was a problem hiding this comment.
I don't remember what this was for but Rajan and I had tried to add some feature like this but I think this is deprecated now.
src/vivarium/component.py
Outdated
| ) | ||
|
|
||
| return builder.lookup.build_table(data) | ||
|
|
||
| def _get_columns( | ||
| self, value_columns: Optional[Sequence[str]], data: float | pd.DataFrame |
stevebachmeier
deleted the
sbachmei/mic-5205/bugfix-lookup-table-non-deterministic-list
branch
Maintain LookupTable value_columns order
Description
Changes and notes
Testing
added a new test and it passes