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

_bearings_distribution: bin_centers terminology #1149

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

dhimmel
Copy link
Contributor

@dhimmel dhimmel commented Mar 16, 2024

This PR addresses terminology in internal-use functions for greater clarity:

  • bin centers were referred to as "edges"
  • bin_counts and bin_centers now have the same length, which is best for most use cases

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.98%. Comparing base (016d3ca) to head (4139a04).

Additional details and impacted files
@@           Coverage Diff           @@
##               v2    #1149   +/-   ##
=======================================
  Coverage   97.98%   97.98%           
=======================================
  Files          24       24           
  Lines        2436     2436           
=======================================
  Hits         2387     2387           
  Misses         49       49           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gboeing
Copy link
Owner

gboeing commented Mar 16, 2024

@dhimmel help me understand what's going on here. Aside from the renaming changes, I see a couple numerical and indexing changes. What's the goal and what are they doing?

@dhimmel
Copy link
Contributor Author

dhimmel commented Mar 16, 2024

I see a couple numerical and indexing changes. What's the goal and what are they doing?

Currently, _bearings_distribution returns (bin_counts, bin_edges) where bin_edges are actually the centers of each bin with the first bin occurring at index position 0 and -1 (because 0 degrees is equivalent to 360 degrees).

The non-renaming changes are entirely related to removing the final repetitive value from the bin centers array. When giving the center of each bin, there is not much rationale for repeating the first bin at the end of the list. The repeated final value was a relic of numpy.histogram's more general design, which in the case of a polar / degree distribution is not needed.

When returning bin_centers, it is nice to have bin_counts be the same length.

osmnx/plot.py Outdated
Comment on lines 759 to 760
# positions: where to center each bar. ignore the last bin edge, because
# it's the same as the first (i.e., 0 degrees = 360 degrees)
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this comment should be updated then.

osmnx/plot.py Outdated
@@ -749,7 +749,7 @@ def plot_orientation( # noqa: PLR0913
}

# get the bearings' distribution's bin counts and edges
Copy link
Owner

Choose a reason for hiding this comment

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

Another reference to edges here.

@dhimmel
Copy link
Contributor Author

dhimmel commented Mar 17, 2024

Given the elegant but potentially confusing implementation of _bearings_distribution with respect to the bin splitting and merging, I revised variable names in this function as well as comments to provide greater clarity. Also added tests to test_bearings

@dhimmel dhimmel requested a review from gboeing March 17, 2024 00:45
@gboeing gboeing merged commit 07839fe into gboeing:v2 Mar 18, 2024
7 checks passed
@gboeing gboeing mentioned this pull request Mar 18, 2024
13 tasks
@dhimmel dhimmel deleted the dsh-bin-centers branch March 18, 2024 01:39
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.

2 participants