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

added option to plot at visual center of fields #94

Merged
merged 9 commits into from
Oct 16, 2023

Conversation

bobmyhill
Copy link
Contributor

@bobmyhill bobmyhill commented Sep 20, 2023

Description

This PR adds the option to plot field labels at the "visual center" of a field, rather than the centroid. The "visual center" is defined as the center of the largest inscribed circle. An option is provided to pass a vertical exaggeration, which instead finds the center of the largest inscribed ellipse.

Builds on #96.

Motivation and Context

For fields with reflex boundary vertices ("concave polygons"), the centroid can plot close to a boundary or even outside the field. The "visual center" of a polygon is often a better location for field labels.

@morganjwilliams: I'm not sure whether this is something you want in the code, but I needed it anyway, so thought I'd open the PR.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@morganjwilliams morganjwilliams added the enhancement New feature or request label Sep 21, 2023
@bobmyhill bobmyhill changed the base branch from main to develop September 21, 2023 08:13
@morganjwilliams morganjwilliams self-assigned this Oct 13, 2023
@morganjwilliams
Copy link
Owner

@bobmyhill given this is adding a dependency, I might see whether it's simpler to just add the relevant code to pyrolite (it looks like the package is just one file, has no dependencies and is MIT licensed...).

@coveralls
Copy link

coveralls commented Oct 13, 2023

Pull Request Test Coverage Report for Build 6510161844

  • 130 of 146 (89.04%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 90.42%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyrolite/plot/spider.py 1 2 50.0%
pyrolite/util/skl/transform.py 0 2 0.0%
pyrolite/util/plot/center.py 113 126 89.68%
Totals Coverage Status
Change from base Build 6505996614: 0.009%
Covered Lines: 6107
Relevant Lines: 6754

💛 - Coveralls

@bobmyhill
Copy link
Contributor Author

@morganjwilliams good point, I've added the code with a reference to the original repo and removed the new dependency. I've also added to the test suite.

@morganjwilliams
Copy link
Owner

Great, thanks @bobmyhill! Will merge it across.

@morganjwilliams morganjwilliams merged commit c3e822b into morganjwilliams:develop Oct 16, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants