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

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Add feature: sector offset #236

merged 5 commits into from
Jul 9, 2024

Conversation

lubyant
Copy link
Contributor

@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
Contributor 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
Contributor 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

windrose/windrose.py Outdated Show resolved Hide resolved
windrose/windrose.py Outdated Show resolved Hide resolved
@lubyant lubyant requested a review from ocefpaf July 4, 2024 03:17
Copy link
Collaborator

@ocefpaf ocefpaf left a comment

Choose a reason for hiding this comment

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

LGTM, I'll leave it here just a bit more to get a second opinion. If no one can look at it in the next week I'll merge it as-is.

Thanks for the PR!

@ocefpaf ocefpaf merged commit fcd9789 into python-windrose:main Jul 9, 2024
15 checks passed
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.

3 participants