[Utilities] Proposal, `time.py`, `format.py` #44

Rubix982 · 2021-07-09T11:28:15Z

[Utilities] Proposal, `time.py`, `format.py`

Pull Request Type

🏆 Enhancements - adds 5 functions as utils

Purpose

Implementing some utilities

Why?

`time.py`

Time utility to convert date format into a UNIX timestamp.

The code to me felt redundant and the quality can be approved, but it works as expected for now, and can manage 6 + 1 formats of the date, making it flexible, starting from Year to Second (not ms).

`format.py`

Functionality for converting some data types into mainly GeoJSON, for merging GeoJSON

This component helps to convert data structure formats. I also added functionality for merging GeoJSONS when building req#2 thinking it might need multiple endpoints - which it didn't in the end, but it is helpful if want to 'join' GeoJSON based on some key in the features.

Feedback required over

A quick pair of 👀 on the code
Discussion on the technical approach

Feedback required when

Whenever feasible

Mentions

@cbeddow feedback over the logic and implementation
@OmarMuhammedAli same as above

The code to me feels redundant and the quality can be approved, but it works as expected for now, and can manage a 6 + 1 formats of the date, making it flexible, starting from Year to Second (not ms)

…N, for merging GeoJSON This component helps to convert data structure formats. I also added a functionality for mergin GeoJSONS when building req#2 thinking it might need multiple end points - which it didn't in the end, but nevertheless, it is helpful if want to 'join' GeoJSON based on some key in the features

gmelodie

This code seems to rewrite a lot of features that are already present in Python itself. Always look for native Python solutions for your problems before implementing your own. Also, if you think a function needs refactoring, do that yourself before submitting the PR. Prioritize quality code over volume

gmelodie · 2021-07-09T12:03:17Z

mapillary/utils/time.py

+    # YYYY-MM-DDTHH:MM:SS
+    match = re.compile(r"[0-9]+-[0-9]+-[0-9]+T[0-9]+:[0-9]+:[0-9]+").match(date)
+
+    if match is not None:


Suggested change

if match is not None:

if not match:

return

gmelodie · 2021-07-09T12:09:30Z

mapillary/utils/time.py

+        if (
+            int(year_month_day[0]) > datetime.datetime.now().year
+            or int(year_month_day[1]) > 12
+            or int(year_month_day[2]) > 31
+            or int(hour_minute_second[0]) > 23
+            or int(hour_minute_second[1]) > 59
+            or int(hour_minute_second[2]) > 59
+        ):
+            raise InvalidDateError(date)


Create a function and call it like is_invalid(year_month_day, hour_minute_second)

gmelodie · 2021-07-09T12:09:46Z

mapillary/utils/time.py

+        if (
+            int(year_month_day[0]) > datetime.datetime.now().year
+            or int(year_month_day[1]) > 12
+            or int(year_month_day[2]) > 31
+            or int(hour_minute_second[0]) > 23
+            or int(hour_minute_second[1]) > 59
+        ):
+            raise InvalidDateError(date)


Function is_invalid

gmelodie · 2021-07-09T12:10:27Z

mapillary/utils/time.py

+        if (
+            int(year_month_day[0]) > datetime.datetime.now().year
+            or int(year_month_day[1]) > 12
+            or int(year_month_day[2]) > 31
+        ):
+            raise InvalidDateError(date)


Function is_invalid (you'll need to adjust the function to work with only one of the two types)

gmelodie · 2021-07-09T12:10:40Z

mapillary/utils/time.py

+    # YYYY-MM-DD
+    match = re.compile(r"[0-9]+-[0-9]+-[0-9]+").match(date)
+
+    if match is not None:


Suggested change

if match is not None:

if not match:

return

gmelodie · 2021-07-09T12:24:32Z

mapillary/utils/format.py

+    geojson_from = geojson_one.copy()
+    geojson_into = geojson_two.copy()


Maybe geojson_src and geojson_dest instead

Changed. Thank you.

gmelodie · 2021-07-09T12:26:37Z

mapillary/utils/format.py

+def merge_geojson(
+    geojson_one: dict,
+    geojson_one_key: str,
+    geojson_two: dict,
+    geojson_two_key: str,
+    debug=True,
+) -> dict:


Add a docstring, it's not evident what it does just by looking at the name (maybe consider changing the name to mutually_update_gojsons or something)

Added in the latest commit.

gmelodie · 2021-07-09T12:27:09Z

mapillary/utils/format.py

+    geojson_one_key: str,
+    geojson_two: dict,
+    geojson_two_key: str,
+    debug=True,


The default value of Debug should not be True

Suggested change

debug=True,

debug=False,

Removed debug all together. For testing purposes, I forgot to remove it.

gmelodie · 2021-07-09T12:34:30Z

mapillary/utils/format.py

+def merge_geojson(
+    geojson_one: dict,
+    geojson_one_key: str,
+    geojson_two: dict,
+    geojson_two_key: str,
+    debug=True,
+) -> dict:
+
+    geojson_from = geojson_one.copy()
+    geojson_into = geojson_two.copy()
+
+    list_one = []
+    list_two = []
+
+    for from_features in geojson_from["features"]:
+
+        for into_features in geojson_into["features"]:
+
+            if (
+                geojson_two_key not in into_features["properties"]
+                or geojson_one_key not in from_features["properties"]
+            ):
+                # * REFACTOR: This is better if it can be logged
+                # * in the future
+                continue
+
+            list_one.append(int(from_features["properties"][geojson_one_key]))
+            list_two.append(int(into_features["properties"][geojson_two_key]))
+
+            if debug:
+                print(
+                    int(from_features["properties"][geojson_one_key])
+                    == int(into_features["properties"][geojson_two_key]),
+                    int(into_features["properties"][geojson_two_key]),
+                    int(from_features["properties"][geojson_one_key]),
+                )
+
+            if int(from_features["properties"][geojson_one_key]) == int(
+                into_features["properties"][geojson_two_key]
+            ):
+
+                old_properties = [key for key in from_features["properties"].keys()]
+
+                new_properties = [key for key in into_features["properties"].keys()]
+
+                if debug:
+                    print(old_properties)
+                    print(new_properties)
+
+                for feature in new_properties:
+
+                    if feature not in old_properties:
+
+                        from_features["properties"][feature] = old_properties[
+                            "properties"
+                        ][feature]
+
+    if debug:
+        print(list(set(list_one) & set(list_two)))
+
+    return geojson_from


I think this entire function could be done using a feature native of Python:

return {**geojson_one, **geojson_two}

Not exactly. I wish to join based on exact ID key matches, similar to an SQL join. Performing return {**geojson_one, **geojson_two} would join all of the keys together and not care for the properties aspect of the GeoJSON

gmelodie · 2021-07-09T12:37:07Z

mapillary/utils/format.py

+                old_properties = [key for key in from_features["properties"].keys()]
+
+                new_properties = [key for key in into_features["properties"].keys()]
+
+                if debug:
+                    print(old_properties)
+                    print(new_properties)
+
+                for feature in new_properties:
+
+                    if feature not in old_properties:
+
+                        from_features["properties"][feature] = old_properties[
+                            "properties"
+                        ][feature]


Suggested change

old_properties = [key for key in from_features["properties"].keys()]

new_properties = [key for key in into_features["properties"].keys()]

if debug:

print(old_properties)

print(new_properties)

for feature in new_properties:

if feature not in old_properties:

from_features["properties"][feature] = old_properties[

"properties"

][feature]

for feature in into_features["properties"].keys():

if feature not in from_features["properties"].keys():

from_features["properties"][feature] = old_properties["properties"][feature]

Rubix982 · 2021-07-09T12:45:32Z

Ayeee, @gmelodie ! Thank you. :D

cbeddow

No technical comments from me, looks like several from @gmelodie for addressing. The format.py I think offers good potential for the future in case other formats are added. May consider leaving room for the future if a user wants to export to a local file in .csv or .shp format, they could import the needed drivers (like via fiona if it were to be brought back) here and then always convert from geojson into the final export format. This is different from the JSON transformations going on here now, of course, but related.

Rubix982 · 2021-07-11T05:57:08Z

@gmelodie your review, please

@OmarMuhammedAli Can I improve this bit of logic even further?

Rubix982 · 2021-07-12T20:14:47Z

@gmelodie review plisssssssssss

mapillary/utils/format.py

refactored to list comprehension for better performance

mapillary/utils/format.py

OmarMuhammedAli

Some logical errors on format.py that I fixed to merge the PR. Great job!

Rubix982 added 2 commits July 9, 2021 16:20

feat: Time utility to convert date format into a UNIX timestamp

0b8e0f2

The code to me feels redundant and the quality can be approved, but it works as expected for now, and can manage a 6 + 1 formats of the date, making it flexible, starting from Year to Second (not ms)

Rubix982 added the utility label Jul 9, 2021

Rubix982 requested review from cbeddow and OmarMuhammedAli July 9, 2021 11:28

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 9, 2021

gmelodie suggested changes Jul 9, 2021

View reviewed changes

cbeddow reviewed Jul 9, 2021

View reviewed changes

Rubix982 added 2 commits July 11, 2021 10:51

Added refatored logic - time.py

58948be

Enhanced merge_geojson documentation, simplify function - format.py

44818e6

Rubix982 added this to Pull Requests - In Progress in MLH Fellowship Jul 11, 2021

Rubix982 mentioned this pull request Jul 11, 2021

[SECURITY - ACCESS_TOKEN] Access Token Introduced, PR#44 #51

Closed

OmarMuhammedAli reviewed Jul 14, 2021

View reviewed changes

mapillary/utils/format.py Outdated Show resolved Hide resolved

minor refactor to image_entities_to_geojson

fdf5239

refactored to list comprehension for better performance

OmarMuhammedAli reviewed Jul 14, 2021

View reviewed changes

mapillary/utils/format.py Outdated Show resolved Hide resolved

OmarMuhammedAli reviewed Jul 14, 2021

View reviewed changes

mapillary/utils/format.py Outdated Show resolved Hide resolved

OmarMuhammedAli added 2 commits July 15, 2021 01:02

fix bug with formatting features list to geojson

c8de043

fix logic of embedding image entities into geojson

d31754f

OmarMuhammedAli approved these changes Jul 14, 2021

View reviewed changes

OmarMuhammedAli merged commit 04ec953 into main Jul 14, 2021

OmarMuhammedAli deleted the Utilities-Proposal-2 branch July 14, 2021 23:33

Rubix982 mentioned this pull request Jul 15, 2021

[Utility] Reformat JSON to GeoJSON #26

Closed

OmarMuhammedAli moved this from Pull Requests - In Progress to Done in MLH Fellowship Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Utilities] Proposal, `time.py`, `format.py` #44

[Utilities] Proposal, `time.py`, `format.py` #44

Rubix982 commented Jul 9, 2021

gmelodie left a comment

gmelodie Jul 9, 2021

gmelodie Jul 9, 2021

gmelodie Jul 9, 2021

gmelodie Jul 9, 2021

gmelodie Jul 9, 2021

gmelodie Jul 9, 2021

Rubix982 Jul 11, 2021

gmelodie Jul 9, 2021

Rubix982 Jul 11, 2021

gmelodie Jul 9, 2021

Rubix982 Jul 11, 2021

gmelodie Jul 9, 2021

Rubix982 Jul 11, 2021

gmelodie Jul 9, 2021

Rubix982 commented Jul 9, 2021

cbeddow left a comment

Rubix982 commented Jul 11, 2021

Rubix982 commented Jul 12, 2021

OmarMuhammedAli left a comment

		geojson_from = geojson_one.copy()
		geojson_into = geojson_two.copy()

[Utilities] Proposal, time.py, format.py #44

[Utilities] Proposal, time.py, format.py #44

Conversation

Rubix982 commented Jul 9, 2021

[Utilities] Proposal, time.py, format.py

Pull Request Type

Purpose

Why?

time.py

format.py

Feedback required over

Feedback required when

Mentions

gmelodie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rubix982 commented Jul 9, 2021

cbeddow left a comment

Choose a reason for hiding this comment

Rubix982 commented Jul 11, 2021

Rubix982 commented Jul 12, 2021

OmarMuhammedAli left a comment

Choose a reason for hiding this comment

[Utilities] Proposal, `time.py`, `format.py` #44

[Utilities] Proposal, `time.py`, `format.py` #44

[Utilities] Proposal, `time.py`, `format.py`

`time.py`

`format.py`