Add two-output graph for visualization.py. Add csv validation for dashboard.#52
Add two-output graph for visualization.py. Add csv validation for dashboard.#52justushelo wants to merge 1 commit intoSimulation-Decomposition:mainfrom
Conversation
…hboard to catch wrong delimiters or incorrect column names. Closes Simulation-Decomposition#45. Closes Simulation-Decomposition#50.
| output_name : str, default "Output 1" | ||
| Name of the primary output variable. | ||
| output_name2 : str, default "Output 2" | ||
| Name of the second output variable. | ||
| xlim : tuple of float, optional | ||
| Minimum and maximum values for the x-axis (Output 1). | ||
| ylim : tuple of float, optional | ||
| Minimum and maximum values for the y-axis (Output 2). |
There was a problem hiding this comment.
To me these are more to set on when working on the figure. This function is supposed to be a minimal block to build something on top as it takes in an ax and returns and ax.
| def visualization( | ||
| *, | ||
| bins: pd.DataFrame, | ||
| bins2: Optional[pd.DataFrame] = None, |
There was a problem hiding this comment.
I would keep this in its own function. A good argument for that is that we have 2 very distinct code paths in a conditional and no shared state between. That means we have 2 different semantics. I would make another function for the 2 output part and if we really wanted to have a single function, then have a higher level wrapper.
There was a problem hiding this comment.
Sounds good👍 I'll make it into a new function.
| ax=None, | ||
| output_name: str = "Output 1", | ||
| output_name2: str = "Output 2", | ||
| xlim: Optional[tuple[float, float]] = None, |
There was a problem hiding this comment.
On optionals, prefer this syntax now xlim: tuple[float, float] | None = None,
| ) | ||
|
|
||
|
|
||
| def _validate_csv_bytes(raw_bytes): |
There was a problem hiding this comment.
Ok but why not just using a big try/except and return a generic explanation of the error if we don't want the raw message from Pandas? In the end we don't have many errors to describe.
Otherwise for the regex, we typically want to compile the regex for performance reasons. Here you also have a re-evaluation in the list comprehension.
There was a problem hiding this comment.
I first tried it with try/excepts but was unable to do it in a way that ensures the dashboard stops the buffering icon. I'll get back to work👍
2 participants
Add two-output option for visualization.py similar to Matlab implementation. Add csv validation to dashboard to catch incorrect delimiters or incorrect column names. Closes #45. Closes #50.