-
Notifications
You must be signed in to change notification settings - Fork 14
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
2018 data update #599
2018 data update #599
Conversation
* AS appeared to add gradle files to app module, unclear if these are actually needed.
* Updated unified.txt * Updated default date to 2018 * NOTE: Still need data fact for 2018
…IP blocks * Requires installing ipaddress python module
* Added geoValidate.py in an attempt to validate script refactor
(Due to large data file upload issues, had to reset my branch, so many commits are squashed into this one) * Removed old code and added comments to geopipeline.py * Removed large raw data files from repo; need better solution for storing these * (iOS) Added 2018 data set, generated unified.txt * (iOS) bumped MAX_DISPLAY_NODES limit * (iOS) Temp crash fix - removed NASA from ASNS_AT_TOP
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 left a few small comments and scanned the python scripts which they seem reasonable. If there is an area in particular that you think warrants more scrutiny I can dig into it further.
Android/app/local.properties
Outdated
@@ -0,0 +1,9 @@ | |||
## This file must *NOT* be checked into Version Control Systems, |
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.
It looks like this file should be gitignored
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.
Removed the gradle setup under the app folder, since it is actually not required. Unclear why it was added during my current work, perhaps I opened the wrong gradle file at some point and it generated the local properties in the wrong location.
No matter, they should now be removed.
@@ -21,7 +21,7 @@ | |||
public class GlobalSettings { | |||
|
|||
// App should default to show this as the default year. | |||
private String defaultYear = "2017"; | |||
private String defaultYear = "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.
It looks like track the default year in at least 3 different places: here, MapController.cpp, and globalSettings.json. If it's not too much work it would be nice to consolidate this data into a single place and reference it from one place. If it is too much work now, filing a separate issue would be good.
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.
Yeah, there is a lot of room for improvement in this process. I will create a ticket for streamlining the data update process and attempt to address it in our next budget iteration. I will make sure to add this comment there.
Data-Pipeline/geo_validate.py
Outdated
@@ -0,0 +1,106 @@ | |||
# | |||
# 2019-02-19 | |||
# geoValidate.py |
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.
There are two similar looking files: geo-validate.py and geoValidate.py. Are both supposed to be checked in?
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 renamed the file, I forgot to update the header.
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.
Nvm, there were actually 2 copies! Good catch I have removed one.
Also, the Nevercode Android build is still failing with this branch. It's not critical to fix that with this PR but if the fix is something small it'd be good to get it in. |
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.
Looks reasonable to me other than the couple things Rob already mentioned.
Also filed #601 to try and get the generation of the data moved out of the app itself at some point. |
Looks like Nevercode is failing due to an older version of Gradle being used. I will update and see if that fixes the build. Edit: Updated the build files to use build tools 28 (required as of November anyways I believe), updated the support libraries and now Nevercode appears to be happy. |
* Unrequired as it is already setup under the main project folder.
* Junit Assert class no longer available; created custom Assert class as a temp solution (should re-evaluate why Asserts are being used in normal app module files)
As of 2019 the data files used to generate the geolocation data has changed. MaxMind no longer hosts the "legacy" databases we were using. Changes were made to use the new GeoLite2 DBs. They are "comparable to, but less accurate than, MaxMind’s GeoIP2 databases."
This meant that updating the data for 2018 also involved digging into the geopipeline python scripts to refactor them to use the available data. I have also created a geo_validate script which attempts to compare two years worth of location data as a way to measure the correctness of my changes. Updated the Data Pipeline README file to indicate these changes.
This PR serves to get the newest data into the application for this development iteration. After complete, and once more time is available we will also need to look into: