Ensure application order is respected in cascade #63
Conversation
Dropping results from the df might lead to df["application"].unique() returning a different list of applications. The order of applications in this list determines the order in which applications are plotted. In order to respect the order of applications in the original DataFrame, and thereby ensure that cascade and navchart plots produced from the same projection use the same legend, we need to ensure that we do not change the application order. Signed-off-by: John Pennycook <[email protected]>
| @@ -30,3 +30,21 @@ def _require_numeric(df, columns): | |||
| except Exception: | |||
| msg = "Column '%s' must contain only numeric values." | |||
| raise TypeError(msg % (column)) | |||
|
|
|||
|
|
|||
| def _sort_by_app_order(df, app_order): | |||
There was a problem hiding this comment.
This function seems really convoluted and I might just be missing the reason why because I don't have a clear mental model for what the dataframe might look like, could you give a really small example of the input/output desired?
I've also never sorted by strings this way before, so it might just be what's required
There was a problem hiding this comment.
You're right to question it. I struggle to make sense of the pandas interface at the best of times, and so a lot of the solutions I come up with are not necessarily the best ones.
If we initially have a DataFrame like this:
| Application | FOM |
|---|---|
| A | 0.5 |
| B | 0.4 |
| B | 0.9 |
| A | 0.7 |
...then df["application"].unique() returns ["A", "B"].
If we filter the original DataFrame such that only the best FOMs remain, then the DataFrame looks like this:
| Application | FOM |
|---|---|
| B | 0.9 |
| A | 0.7 |
...and df["application"].unique() returns ["B", "A"].
So what this function does is accept the second (post-filtering) DataFrame and a list denoting the desired application order (i.e., ["A", "B"]), and then sorts the DataFrame such that the results appear in the same order:
| Application | FOM |
|---|---|
| A | 0.7 |
| B | 0.9 |
Note that this isn't the same as sorting the application names. What we're trying to do is ensure that we don't alter the order in which applications appear in the DataFrame, in case that order is meaningful to the user.
As a concrete example... Imagine that the results in the DataFrame are all sorted by collection date, and that a user is appending new results to the DataFrame as they're collected. Without this convoluted sorting, adding a new (better) result to the bottom of the DataFrame could change the order in which applications appear, resulting in a different legend the next time the graph is plotted.
Does that make sense?
There was a problem hiding this comment.
Inspired by our conversation about "doing one thing well" on another pull request, I think I'm coming around to the view that #22 probably was a mistake. I'll sketch out an alternative fix and open that as another pull request, so we can compare the two possible behaviors.
Plotting a NavChart requires a calculation of performance portability, which may adjust the order of applications in the DataFrame. Signed-off-by: John Pennycook <[email protected]>
The previous "expected" test result had actually been chosen based on the empirical behavior of the library. If we expect the output of the DataFrame to remain sorted by application, we should test for that. Signed-off-by: John Pennycook <[email protected]>
|
Closed in favor of #65. |
Dropping results from the
dfmight lead todf["application"].unique()returning a different list of applications. The order of applications in this list determines the order in which applications are plotted.In order to respect the order of applications in the original DataFrame, and thereby ensure that cascade and navchart plots produced from the same projection use the same legend, we need to ensure that we do not change the application order.
Related issues
This was broken by #22. We didn't pick it up until now because the graphs we've been producing aren't strictly wrong -- it's hard to see the issue without plotting a cascade plot and navchart side-by-side.
@swright87, I don't know if the PGFplots backend already handles this correctly, or if it requires an equivalent fix. Please take a look.
Proposed changes
There might be a more efficient way to do this that doesn't require sorting the DataFrame, but the
_sort_by_app_orderfunction already existed elsewhere in the code base, making this an expedient fix.It may also be worth discussing if this is actually the fix we want, or if we applied #22 too hastily. If we instead threw an error whenever there were duplicate results in a DataFrame (and required the user to remove them somehow) there would be no chance of this happening.