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

[STRAT-114] Route Model Refactor- Elevations Finalized #93

Merged
merged 2 commits into from
Dec 5, 2020

Conversation

krishna-kalavadia
Copy link
Member

Elevations file completed. Also regenerated the Heartland Elevations because the last one had errors so that is also included in this PR, so only two files.

e-wai
e-wai previously approved these changes Dec 2, 2020
Copy link
Collaborator

@e-wai e-wai left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can I assume that tests and speedlimit are a later issue?

for key in pair.keys():
# Step 2: Multiply each value by 100000 and round each result to the nearest int
new_lat = int(round(float(key) * 100000))
new_long = int(round(float(pair[key]) * 100000))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could swap pair[key] with value if you change line 34 to for key, value in pair.items()

new_long = int(round(float(pair[key]) * 100000))

# Step 3: Calculate the difference between every pair of values
dx = (new_lat - lat)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry this is small, but don't think the brackets do anything for readability

# each remainder, stop when the quotient reaches zero
rem = []
while index > 0:
rem.append(int(index % 32))
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you start with an int, think the int cast call is redundant

rem = []
while index > 0:
rem.append(int(index % 32))
index = int(index / 32)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can replace with index // 32

routemodel/data_retrieval/elevations.py Outdated Show resolved Hide resolved
sys.exit()
# append specified elevation URL details
url = BASE_URL + 'Elevation/Polyline?'
sample = str(sample_val)
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think you need the string cast; can just put it directly into format?

# loop through coordinates and write coordinates into DataFrame
counter = 0
for index, pair in enumerate(coordinates):
for key in pair.keys():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as first comment

@ufshk
Copy link
Member

ufshk commented Dec 2, 2020

Looks good to me. Can I assume that tests and speedlimit are a later issue?

Yes, yes you can.

@krishna-kalavadia krishna-kalavadia changed the title [STRAT-???] Route Model Refactor- Elevations Finalized [STRAT-114] Route Model Refactor- Elevations Finalized Dec 3, 2020
Copy link
Collaborator

@e-wai e-wai left a comment

Choose a reason for hiding this comment

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

Looks really good to me 🥳 Really appreciate how you built everything

@ufshk have you reviewed too? do you have any thoughts on separating the get request into its own method for testing?

Copy link
Collaborator

@mdshiozaki mdshiozaki left a comment

Choose a reason for hiding this comment

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

Looks solid to me, will wait to see about that comment on potentially moving the get request for testing but thank you!

@krishna-kalavadia
Copy link
Member Author

krishna-kalavadia commented Dec 5, 2020

Looks really good to me 🥳 Really appreciate how you built everything

@ufshk have you reviewed too? do you have any thoughts on separating the get request into its own method for testing?
Looks solid to me, will wait to see about that comment on potentially moving the get request for testing but thank you!

Thanks for taking the time to review this!

@e-wai e-wai merged commit 44b93de into uw-midsun:master Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants