220 df index sort mgtfs #249
Conversation
There was a problem hiding this comment.
Thanks for your work on this @CBROWN-ONS. All seems to work as intended. There's only a couple of actions to take:
- Unit tests for
_summary_col_sorter. - Corrections to type hint and docstring for
summarise_trips.
The rest are suggestions/clarifications or maybe misunderstandings on my part. Happy to discuss any of the points!
| trip_counts = self.instances[0]._order_dataframe_by_day(trip_counts) | ||
| if sort_by == "route_type": | ||
| trip_counts = trip_counts.sort_values("route_type") | ||
| if to_days: |
There was a problem hiding this comment.
Note that if to_days is set to True, the index after sorting will be messed up. This is nitpicking, but perhaps we could add a reset_index somewhere.
There was a problem hiding this comment.
Good shout. Added this
There was a problem hiding this comment.
I've changed the reset_index to the return as this wasn't fixing the issue when to_days was True. Let me know if you're not happy with this!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #249 +/- ##
==========================================
+ Coverage 97.71% 98.16% +0.44%
==========================================
Files 21 21
Lines 1838 1849 +11
==========================================
+ Hits 1796 1815 +19
+ Misses 42 34 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi @SergioRec . I've made some changes based on your comments 😄 . |
There was a problem hiding this comment.
Thanks for addressing all my comments @CBROWN-ONS.
I've made a small change to the location of the reset_index in _summarise_core. Could you please check if you're happy with it. Other than that, everything else works and this PR can be merged. Thanks!
|
Looks good to me @SergioRec . Happy for this to be merged |
* feat: sorting params for mgtfs summaries * feat: additional tests for new sorting features * fix: restructured param to remove unnecessary sort options * fix: add reset index after sorting by route type * fix: update docstring to address sorting conditions * fix: moved reset_index to return statement --------- Co-authored-by: Sergio Recio <[email protected]> bf5cabd
Description
Adds additional sorting options for mgtfs summaries
Fixes #220
Motivation and Context
Type of change
How Has This Been Tested?
Test configuration details:
Advice for reviewer
Checklist:
Additional comments