-
Notifications
You must be signed in to change notification settings - Fork 2
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
DATASET: Flood risk data #313
Conversation
Codecov Report
@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 82.58% 84.47% +1.88%
==========================================
Files 87 86 -1
Lines 2665 2596 -69
Branches 274 254 -20
==========================================
- Hits 2201 2193 -8
+ Misses 401 340 -61
Partials 63 63 |
009157e
to
007d383
Compare
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.
Some small suggestions to make the area it applies to clear.
Would be good to have documentation somewhere of how the data was derived and what it means. My assumption here is that for each constituency there's a per $some-sub-area flood risk rating that we are aggregating into these percentages but that's only a guess from how we're turning the data into numbers.
<tr> | ||
<th scope="col">Risk</th> | ||
<th scope="col" data-color="#068670">This area</th> | ||
<th scope="col" data-color="#ced4da">UK average</th> |
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.
This should be England not UK?
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.
This is an issue for a number of datasets that are England or England+Wales only. I figure it’s something we chalk up to fix later, rather than holding up this dataset import.
message = "Importing constituency flood risk data" | ||
defaults = { | ||
"label": "Flood risk from rivers or sea", | ||
"description": "Data relating to risk of flooding from rivers or sea, recorded in 2018", |
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.
probably worth adding something to these to make it clear it's for england only, maybe just add "in England" after sea, or "for England" after Data.
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’d be tempted to put it into the label – "Flood risk from rivers or sea (England only)"
Also, a change to the _place_data template that means that datasets with no actual data in them do not render empty cards
007d383
to
90d68db
Compare
Fixes #195
Flood risk data csv produced using qgis, file can be found in shared google drive.
Also, a change to the _place_data template that means that datasets with no actual data in them do not render empty cards