Skip to content
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

HTML outputs for GTFS data. #112

Merged
merged 40 commits into from
Sep 15, 2023
Merged

HTML outputs for GTFS data. #112

merged 40 commits into from
Sep 15, 2023

Conversation

CBROWN-ONS
Copy link
Collaborator

Description

closes #47
closes #59
closes #48
closes #46
closes #45
closes #49

Motivation and Context

This PR intends to allow for HTML reports to be created on GTFS datasets. The key functionality added through this PR is the gtfs::validation::GtfsInstance().html_report() method.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Test configuration details:

  • OS: Windows 10
  • Python version: 3.9.13
  • Java version: N/A
  • Python management system: conda

Advice for reviewer

Checklist:

  • My code follows the intended structure of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional comments

@CBROWN-ONS CBROWN-ONS added enhancement New feature or request GTFS labels Aug 31, 2023
@CBROWN-ONS CBROWN-ONS requested a review from r-leyshon August 31, 2023 12:06
@CBROWN-ONS CBROWN-ONS changed the title Gtfs html new HTML Outputs for GTFS data. Aug 31, 2023
@CBROWN-ONS CBROWN-ONS changed the title HTML Outputs for GTFS data. HTML outputs for GTFS data. Aug 31, 2023
@r-leyshon r-leyshon self-assigned this Sep 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

Patch coverage: 94.87% and project coverage change: +2.41% 🎉

Comparison is base (79f947c) 84.86% compared to head (93545dc) 87.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #112      +/-   ##
==========================================
+ Coverage   84.86%   87.27%   +2.41%     
==========================================
  Files          10       11       +1     
  Lines         674      896     +222     
==========================================
+ Hits          572      782     +210     
- Misses        102      114      +12     
Flag Coverage Δ
unittests 87.27% <94.87%> (+2.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/transport_performance/gtfs/validation.py 96.61% <92.85%> (-3.39%) ⬇️
src/transport_performance/gtfs/gtfs_utils.py 100.00% <100.00%> (ø)
.../transport_performance/gtfs/report/report_utils.py 100.00% <100.00%> (ø)
src/transport_performance/gtfs/routes.py 93.33% <100.00%> (ø)
src/transport_performance/osm/osm_utils.py 75.75% <100.00%> (ø)
src/transport_performance/utils/defence.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 723 to 736
def test__plot_route_summary_on_pass(self, gtfs_fixture):
"""Test plot_route_summary() calls _plot_summary() succesfully."""
gtfs_fixture.summarise_routes()
assert isinstance(
gtfs_fixture.plot_route_summary(target_summary="mean"),
PlotlyFigure,
), "plot_route_summary() failed to return plotly figure"

def test__plot_trip_summary_on_pass(self, gtfs_fixture):
"""Test plot_trip_summary() calls _plot_summary() succesfully."""
gtfs_fixture.summarise_trips()
assert isinstance(
gtfs_fixture.plot_trip_summary(target_summary="mean"), PlotlyFigure
), "plot_trip_summary() failed to return plotly figure"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are clones of each other, testing the same logic in the src code that I suggested would be better refactored to one method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CBROWN-ONS This issue with the lint being written out to directory. This file has been mistakenly committed and the test is writing out directories / css files (interestingly, the css file doesn't always appear though).
Uncertain what the cause is as your use of tmp_pth looks fine. May need to default to tmpdir if we can't get tmp_pth to behave as needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pushed up the removal of this lint and re-running. Problem seems to have gone away for me. If you could confirm on your end? Just run the pytest (maybe coverage run -m pytest too just to be sure) and confirm that no lint is being written out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@r-leyshon. It has now been removed from the branch completely. I can confirm that it isn't present and that it also does not impact any of the tests (the test suite is running smoothly).
I think it was a remnant of an older test I had implemented that had since been removed.
All seems fine on my end now

Copy link
Contributor

@r-leyshon r-leyshon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All conversations resolved. New issues arising from this PR:
#129
#130
#131
#133
#134

@r-leyshon r-leyshon merged commit 8d973c4 into dev Sep 15, 2023
@r-leyshon r-leyshon deleted the gtfs-html-new branch September 15, 2023 14:08
github-actions bot pushed a commit that referenced this pull request Sep 15, 2023
* Add code for producing HTML outputs for GTFS data

* Update requirements.txt

* fix: Update tests to reflect changes

* Added better file ext checking; update tests; added new tests

* chore: Clean up docstrings to comply with numpy formatting

* chore: Add additional raises in docstrings

* chore: Breaking changes with dev resolved

* refactor: Several type utils -> _type_defence

* fix: Undo accidental overwrite with review branch

* test: _type_defence raises with single or multiple types

* test: _type_defence pass on single & multiple values to types

* Refactor save paths for plotting summaries (#124)

* feat: Tests for _check_column_in_df()

* fix: Added another stage of testing for set_up_report_dir() which improves coverage

* Updated gtfs_utils.py and corresponding tests. Removed scheme param. Improved type check. Improved type hint. Included test fixture

* Make TemplateHtml methods private; Update docstring

* Updated error message for replace_multiple=False and multiple placeholders found

* Updated test_report_utils.py to reflect private functions

* Updated _set_up_report_dir() to better suite check_parent_dir_exists() functionality

* Update tests in test_report_utils

* Only .lower() orientation in one place, rather than 2

* Create small helper function to remove duplicate logic; added testing

* Move test to test__plot_summary_defences() as it is more appropriate

* Refactore validation.py to only use _plot_summary();Added additional defences;Included and updated testing for all changes/additions

* Update check_attribute() due to save error

* Update defence.py

* Update defence.py for missing char

* chore: Run pre-commit against all files

* docs: Update private methods in docstring

* chore: Remove lint

* refactor: Param formatting in Error message

* refactor: Rm str replace carriage returns logic in test

* chore: Remove filterwarning

---------

Co-authored-by: r-leyshon <richard.leyshon@ons.gov.uk> 8d973c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment