-
Notifications
You must be signed in to change notification settings - Fork 133
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
Fix issue where sectors showed a straight line instead of a curved #266
Conversation
I added 5102055 because I have a hard time running the tests locally. There is always something with the fonts being slightly different. And it's hard to know why a test failed in CI if you cannot get the comparisons. |
@@ -38,3 +38,10 @@ jobs: | |||
run: | | |||
pytest -s -rxs -vv -Werror tests/ --mpl --mpl-generate-summary=html \ | |||
--mpl-results-path="windrose_test_output-${{ matrix.os }}-${{ matrix.python-version }}" | |||
- name: Store mpl-results | |||
uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -642,6 +642,8 @@ def bar(self, direction, var, **kwargs): | |||
zorder=zorder, | |||
**kwargs, | |||
) | |||
# needed so the the line of the rectangle becomes curved | |||
patch.get_path()._interpolation_steps = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, if I understand this correctly, this is a hack b/ it is using a private method to please our eyes with something that did not looked curved due to low resolution but was "correct." With that said, I do prefer this behavior even if it is prone to break with future changes in mpl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 100% agree on that being not optimal and originally private, but since it's even in the docs I think that's the only way to get there?
I still do think a straight line is technically incorrect in a polar projection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do think a straight line is technically incorrect in a polar projection.
For our eyes? Yes. I agree. But in theory the "data" is correct otherwise the higher interpolation steps would still produce a straight line, right?
Anyway, I did not mean that this was the problem as for a graph what we see is the important part. I think that mpl is probably choosing non-ideals defaults for us in this case.
resolves #137
I think I found the solution by looking at what the usual
.bar()
does. We need to set._interpolation_steps
to a value > 1 (https://matplotlib.org/stable/api/projections/polar.html#matplotlib.projections.polar.PolarTransform).I updated the baseline images affected, since we were (sometimes) initially testing the "wrong" behavior - there was also some test pollution going on making some plots curved others straight.
I think the only correct behavior are curved lines because in a polar coordinate system straight lines would mean varying values depending on where you read the value from.
Happy to hear your thoughts on this