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

all code/utils.py to core/* && evaluate_h3cells overhaul #69

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

jGaboardi
Copy link
Collaborator

@jGaboardi jGaboardi commented Jun 17, 2024

This is a hefty PR that:

@martinfleis
Copy link
Contributor

I will let @anastassiavybornova review this one as it mostly touches her code.

@jGaboardi
Copy link
Collaborator Author

I will let @anastassiavybornova review this one as it mostly touches her code.

LOL Okay lazy bones.

@jGaboardi
Copy link
Collaborator Author

jGaboardi commented Jun 17, 2024

@martinfleis I think we'll actually want your eyes on a couple things for performance that @anastassiavybornova had mentioned when she made her original PR for this stuff. While I have reproduced her outputs (mostly) faithfully with some improved performance there is still the outstanding use of apply in cell 10 of evaluate_h3cells.ipynb as it relates to get_edge_stats() and get_edge_stats() in core/stats.py

@martinfleis
Copy link
Contributor

It looks okay to me. Is there some performance issue? Does it take hours or anything?

@jGaboardi
Copy link
Collaborator Author

It looks okay to me. Is there some performance issue? Does it take hours or anything?

OK, cool. For Auckland that notebook cell takes ≈2 seconds to run. I wasn't sure if there was some easy adjustment or clever spatial join that could be done to cut that time. At any rate, I think good enough for now and we can revisit if it becomes a problem with larger AOIs.

@martinfleis
Copy link
Contributor

If it took 20 minutes I would hesitate if it is worth any further attention. At 2s, you spend more reading this comment than what would you save in the end.

Copy link
Collaborator

@anastassiavybornova anastassiavybornova 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, thanks James for the housekeeping as always <3 and yes sure, agree for the performance, if there's no palpable issue let's just leave the .apply (well, now it's .map) inside the stats functions

@jGaboardi jGaboardi merged commit 4b495ae into main Jun 19, 2024
12 checks passed
@jGaboardi jGaboardi deleted the GH62_annas_stuff_hex_etc branch June 19, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants