-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support keyword-only arguments #116
Changes from 1 commit
24452df
36b4695
965d212
254ad85
fa9ce4b
1cf8184
686e27f
58b3b00
0311569
ba11f3d
386e014
357866a
8919135
9a7b2b5
8dcaa82
6f10f90
1d16e5f
5810cc4
1382afd
011f9bf
f24a62f
b7a160e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -418,12 +418,12 @@ def set_param_table(self, params: ParamTable) -> None: | |
for index, label in zip(params.index, values): | ||
self._set_provider( | ||
Item((Label(tp=params.row_dim, index=index),), param_name), | ||
Provider.table_row(label), | ||
Provider.table_label(label), | ||
) | ||
for index, label in zip(params.index, params.index): | ||
self._set_provider( | ||
Item((Label(tp=params.row_dim, index=index),), params.row_dim), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessary, but could this clumsy key setup be absorbed into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you store the key in the provider? This would duplicate the key as we still need it in the graph / pipeline. |
||
Provider.table_row(label), | ||
Provider.table_label(label), | ||
) | ||
|
||
def del_param_table(self, row_dim: type) -> None: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why label? I thought that would refer to columns, or it at least feels unclear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is what it's called in the code:
sciline/src/sciline/pipeline.py
Line 421 in f24a62f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but shouldn't that be renamed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know. Pipeline seems to use 'label' and 'index' interchangeably for tables. So we could rename it to index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is two different things. If you rename it to index it would mean an entry of the index column:
pandas.DataFrame
(and it also has a column label, the type of the index).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You renamed the method to
table_cell
, but here it is stilltable_label
.