-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix and change table sorting in Python #903
Changes from 1 commit
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 |
---|---|---|
|
@@ -82,6 +82,8 @@ class Terminal(NodeModel): | |
default_factory=TableModel[TerminalStaticSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"static": ["node_id"]} | ||
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. I don't agree with adding this protected variable in multiple places. There are other ways to do this. For example, add an But I am trying to look a bit further. It would be better if we receive this information from the julia code. For example, by adding it in the schemas and reading the schemas in python. Would that be something that is possible with pydantic? 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.
If possible I agree it would be nice to share this or run this through pydantic. I'd like to keep this PR focused on a bugfix and not add a refactor on top. 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. 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. I have been playing a bit more with this, and it seems to me that the best option would be to make the sort_keys an instance Field sort_keys: list[str] = Field(init_var=True, exclude=True, repr=False) I set it to init_var, so it can never be changed afterwards, and I set it to exclude, because the data is always set in python. The only problem that I still had was the moment that you set a pandas DataFrame, because then it tells me that:
I believe we could solve this by using a different scheme for custom data types, instead of 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. Is init_var=True needed? Because if you set a DataFrame, the TableModel already exists, including its specific sort_keys. Ideally we get that from somewhere.
Fully agree, but I couldn't find a way of doing so. It requires some custom info passed to the JSON Schema, the https://github.com/koxudaxi/datamodel-code-generator/, to read it and pass it into some class as a private field, or Config option, that we here can pick up on. 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. I created #956 for the first effort on getting the sort_keys in pydantic. You could decide to enhance the ticket, or otherwise just make a new ticket to get the info from Julia. |
||
|
||
|
||
class PidControl(NodeModel): | ||
static: TableModel[PidControlStaticSchema] = Field( | ||
|
@@ -91,7 +93,10 @@ class PidControl(NodeModel): | |
default_factory=TableModel[PidControlTimeSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"time": ["time", "node_id"]} | ||
_sort_keys: dict[str, list[str]] = { | ||
"static": ["node_id", "control_state"], | ||
"time": ["node_id", "time"], | ||
} | ||
|
||
|
||
class LevelBoundary(NodeModel): | ||
|
@@ -102,14 +107,19 @@ class LevelBoundary(NodeModel): | |
default_factory=TableModel[LevelBoundaryTimeSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"time": ["time", "node_id"]} | ||
_sort_keys: dict[str, list[str]] = { | ||
"static": ["node_id"], | ||
"time": ["node_id", "time"], | ||
} | ||
|
||
|
||
class Pump(NodeModel): | ||
static: TableModel[PumpStaticSchema] = Field( | ||
default_factory=TableModel[PumpStaticSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"static": ["node_id", "control_state"]} | ||
|
||
|
||
class TabulatedRatingCurve(NodeModel): | ||
static: TableModel[TabulatedRatingCurveStaticSchema] = Field( | ||
|
@@ -119,8 +129,8 @@ class TabulatedRatingCurve(NodeModel): | |
default_factory=TableModel[TabulatedRatingCurveTimeSchema] | ||
) | ||
_sort_keys: dict[str, list[str]] = { | ||
"static": ["node_id", "level"], | ||
"time": ["time", "node_id", "level"], | ||
"static": ["node_id", "control_state", "level"], | ||
"time": ["node_id", "time", "level"], | ||
} | ||
|
||
|
||
|
@@ -144,7 +154,10 @@ class FlowBoundary(NodeModel): | |
default_factory=TableModel[FlowBoundaryTimeSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"time": ["time", "node_id"]} | ||
_sort_keys: dict[str, list[str]] = { | ||
"static": ["node_id"], | ||
"time": ["node_id", "time"], | ||
} | ||
|
||
|
||
class Basin(NodeModel): | ||
|
@@ -165,8 +178,9 @@ class Basin(NodeModel): | |
) | ||
|
||
_sort_keys: dict[str, list[str]] = { | ||
"static": ["node_id"], | ||
"profile": ["node_id", "level"], | ||
"time": ["time", "node_id"], | ||
"time": ["node_id", "time"], | ||
"subgrid": ["subgrid_id", "basin_level"], | ||
} | ||
|
||
|
@@ -176,6 +190,8 @@ class ManningResistance(NodeModel): | |
default_factory=TableModel[ManningResistanceStaticSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"static": ["node_id", "control_state"]} | ||
|
||
|
||
class DiscreteControl(NodeModel): | ||
condition: TableModel[DiscreteControlConditionSchema] = Field( | ||
|
@@ -185,20 +201,29 @@ class DiscreteControl(NodeModel): | |
default_factory=TableModel[DiscreteControlLogicSchema] | ||
) | ||
|
||
# TODO complete the ordering, and make sure it aligns with truth_state | ||
_sort_keys: dict[str, list[str]] = {"static": ["node_id"], "logic": ["node_id"]} | ||
deltamarnix marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class Outlet(NodeModel): | ||
static: TableModel[OutletStaticSchema] = Field( | ||
default_factory=TableModel[OutletStaticSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"static": ["node_id", "control_state"]} | ||
|
||
|
||
class LinearResistance(NodeModel): | ||
static: TableModel[LinearResistanceStaticSchema] = Field( | ||
default_factory=TableModel[LinearResistanceStaticSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"static": ["node_id", "control_state"]} | ||
|
||
|
||
class FractionalFlow(NodeModel): | ||
static: TableModel[FractionalFlowStaticSchema] = Field( | ||
default_factory=TableModel[FractionalFlowStaticSchema] | ||
) | ||
|
||
_sort_keys: dict[str, list[str]] = {"static": ["node_id", "control_state"]} |
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.
I reordered the columns such that the first entry is the top-level sort, and lower level sorts are followed, which I hope will make it easier to interpret the sorting from these tables.