Support param tables with only index and no columns #106
Conversation
| existing == params.row_dim | ||
| and param_name in self._param_tables[existing] | ||
| ): | ||
| # Column will be removed by del_param_table below, clash is ok |
There was a problem hiding this comment.
Can you ever be in a situation where an error is raised in the loop and you never get to the del_param_table, and somehow end up with a corrupt pipeline that you cannot use?
I guess the risk is minimal and you would just re-build the pipeline from scratch instead of trying to fix it?
There was a problem hiding this comment.
Not sure I understand your question. This check is moved here (from below) precisely to avoid corruption, i.e., before any modification?
There was a problem hiding this comment.
This check is moved here (from below)
Where was it moved from exactly?
There was a problem hiding this comment.
The if params.row_dim in self._param_tables: that used to be above is now below these lines.
|
|
||
| pl = sl.Pipeline((to_str,)) | ||
| pl.set_param_series(int, ints) | ||
| assert pl.compute(sl.Series[int, str]) == sl.Series(int, {1: '1', 2: '2', 3: '3'}) |
There was a problem hiding this comment.
Do we need a test to check if someone is trying to use set_param_series on a pipeline that already has a param_table with different columns? Or maybe that is just handled by the fact that set_param_series is calling set_param_table internally?
There was a problem hiding this comment.
Yes that is handled.
Fixes #58.
Also fixes #105. We support replacing tables, but each table is treated atomically, i.e., we do not support replacing a column in an existing table when adding another table.