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

Add feature: sector offset #236

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lubyant
Copy link

@lubyant lubyant commented Apr 22, 2023

Apply for the issue #235
Add an argument for the method such that the sector can be edited flexibly.
For example

Picture1

By default (left), the first sector is [-11.25, 11.25].

dir = [360*random.random() for _ in range(400)]
spd = [5*random.random() for _ in range(len(dir))]
ax.bar(dir, spd, bins=bins, nsector=16)

Create an 11.25 offset for the sector (right), the first sector is [0, 22.5] and also for the other sectors. And, the bar is rotate 11.25 degrees.

dir = [360*random.random() for _ in range(400)]
spd = [5*random.random() for _ in range(len(dir))]
ax.bar(dir, spd, bins=bins, nsector=16, sectoroffset=11.25)

The same thing applies to the box plot,
Picture2

@lubyant lubyant mentioned this pull request Apr 22, 2023
@ocefpaf
Copy link
Collaborator

ocefpaf commented Jun 12, 2023

The code is solid and does what you need.

I'm not an expert here so can you explain a bit more on the use cases for this change? If we cannot get more experienced devs here to review I'm tempted to merge b/c this doesn't disrupt the conventional use anyway.

@akrherz
Copy link

akrherz commented Jun 12, 2023

This change appears to be a convenient way to enforce the "engineering" convention for windroses whereby the bars represent the direction the wind is blowing toward and not from (meteorology). Reviewing the PR, I do notice a spelling typo with offsect

@lubyant
Copy link
Author

lubyant commented Jun 12, 2023

I made this pr because sometime I need to define the sector starting from 0, not -11.25 (for 16 sectors for example). I modify the setting to adapt this.
However, I found that I might not correctly switch the histogram. Currently, I just simply switch the rose bar but I don't calculate the histogram based on new sectors. Please don't merge this pr. I will submit another pr to fix this issue.

@lubyant
Copy link
Author

lubyant commented Jun 17, 2023

Hi @ocefpaf,
I have made a few changes to incorporate the change in the histogram. Here is what it looks like. Can you review my pull request?
image
image

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.

None yet

3 participants