-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Provide adjustable='box' to 3D axes aspect ratio setting
#23552
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
Conversation
adjustable='box' to 3D axes aspect ratio setting (追記ここまで)
I think that it may be possible to factor out
if aspect == 'equal':
ax_indices = [0, 1, 2]
elif aspect == 'equalxy':
ax_indices = [0, 1]
elif aspect == 'equalxz':
ax_indices = [0, 2]
elif aspect == 'equalyz':
ax_indices = [1, 2]
and then do something like:
ax_indices = _get_ax_indices(aspect)
old_diag = np.norm([box_aspect[a] for a in ax_indices]) # Maybe better way to pick out values?
new_diag = np.norm([deltas[a] for a in ax_indices]) # Same
for i in ax_indices:
box_aspect[i] = deltas[i]
remaining_index = {0, 1, 2} - set(ax_indices)
if remaining_index:
box_aspect[remining_index.pop()] = new_diag/old_diag
(I take it that numpy may have a better way to do the index fiddling.)
Ahh, I didn't see the indentation level correctly. And it seems like you can get away with simpler indexing, something like:
old_diag = np.norm(box_aspect[ax_indices])
new_diag = np.norm(deltas[ax_indices])
box_aspect[ax_indices] = deltas[ax_indices]
remaining_index = {0, 1, 2} - set(ax_indices)
if remaining_index:
box_aspect[remining_index.pop()] = new_diag/old_diag
Looks promising! I think a test, update of documentation (right now it says that it is ignored), a release note, and an argument check (similar to the aspect test, using _api.check_in_list) and one can review it properly.
For test, I guess that reusing the same aspect test, but with adjustable='box' may be a good and easy way.
I think that it may be possible to factor out
I've got this factored out here at the moment, we can settle on whatever.
Turns out that the variable was already available at that point in the code. Bad read-up on my side.
I think that it may be possible to factor out
I've got this factored out
This is brilliant; I'll rebase once it gets merged.
I think that it may be possible to factor out
I've got this factored out
This is brilliant; I'll rebase once it gets merged.
I expect that MR to take a while to get reviewed and go through so don't let it hold you back on this one! :) I can handle the merge conflict over there.
aa5228b to
2a702cf
Compare
This is great! Looks good to me now.
Doc build has failed. I think this is the error (since the log says that 'build finished with problems, 1 warning'):
/home/circleci/project/doc/users/next_whats_new/font_fallback.rst:7: WARNING: Exception occurred in plotting font_fallback-1
from /home/circleci/project/doc/users/next_whats_new/font_fallback.rst:
Traceback (most recent call last):
File "/home/circleci/project/lib/matplotlib/sphinxext/plot_directive.py", line 644, in render_figures
figman.canvas.figure.savefig(img.filename(fmt), dpi=dpi)
File "/home/circleci/project/lib/matplotlib/figure.py", line 3197, in savefig
self.canvas.print_figure(fname, **kwargs)
File "/home/circleci/project/lib/matplotlib/backend_bases.py", line 2339, in print_figure
result = print_method(
File "/home/circleci/project/lib/matplotlib/backend_bases.py", line 2205, in <lambda>
print_method = functools.wraps(meth)(lambda *args, **kwargs: meth(
File "/home/circleci/project/lib/matplotlib/_api/deprecation.py", line 410, in wrapper
return func(*inner_args, **inner_kwargs)
File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 520, in print_png
self._print_pil(filename_or_obj, "png", pil_kwargs, metadata)
File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 466, in _print_pil
FigureCanvasAgg.draw(self)
File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 408, in draw
self.figure.draw(self.renderer)
File "/home/circleci/project/lib/matplotlib/artist.py", line 74, in draw_wrapper
result = draw(artist, renderer, *args, **kwargs)
File "/home/circleci/project/lib/matplotlib/artist.py", line 51, in draw_wrapper
return draw(artist, renderer)
File "/home/circleci/project/lib/matplotlib/figure.py", line 2985, in draw
mimage._draw_list_compositing_images(
File "/home/circleci/project/lib/matplotlib/image.py", line 131, in _draw_list_compositing_images
a.draw(renderer)
File "/home/circleci/project/lib/matplotlib/artist.py", line 51, in draw_wrapper
return draw(artist, renderer)
File "/home/circleci/project/lib/matplotlib/text.py", line 688, in draw
bbox, info, descent = self._get_layout(renderer)
File "/home/circleci/project/lib/matplotlib/text.py", line 322, in _get_layout
w, h, d = _get_text_metrics_with_cache(
File "/home/circleci/project/lib/matplotlib/text.py", line 97, in _get_text_metrics_with_cache
return _get_text_metrics_with_cache_impl(
File "/home/circleci/project/lib/matplotlib/text.py", line 105, in _get_text_metrics_with_cache_impl
return renderer_ref().get_text_width_height_descent(text, fontprop, ismath)
File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 242, in get_text_width_height_descent
font.set_text(s, 0.0, flags=get_hinting_flag())
File "/home/circleci/project/lib/matplotlib/_text_helpers.py", line 16, in warn_on_missing_glyph
_api.warn_external(
File "/home/circleci/project/lib/matplotlib/_api/__init__.py", line 363, in warn_external
warnings.warn(message, category, stacklevel)
UserWarning: Glyph 20960 (\N{CJK UNIFIED IDEOGRAPH-51E0}) missing from current font.
which isn't quite clear to me. I don't suppose modifying 3d_plot_aspects.rst caused this somehow?
The docs font fallback issue is unrelated to your PR, try rebasing onto main and seeing if it works when it runs again.
d9518e0 to
75e605f
Compare
It looks like this didn't get reviewed in time to make the cut for 3.6, which won't have an effect except that the what's new might need to be rewritten.
How does it work—will it be enough to just create a new "What's New" file?
Actually, no changes are required for the what's new. The content and structure is version agnostic. When the PR gets merged the content is put int the next_whats_new folder on the main branch, and that will be picked up in the next release.
@oscargus you have started looking into this. Could you give a second review?
Considering that this PR modifies a what's new note from 3.6, I believe that something should be done.
I think the PR is good to go, subject to the what's new note, so will not merge at this stage.
Could you also take a look at #23552 (comment).
76ab0f0 to
57f2b5a
Compare
For the "What's New" entry, I used scottshambaugh's original example, but with slightly different dimensions of the box (as in the test), which I think highlight the differences nicely.
Looks good! Thanks!
...b#23552) * Provided `adjustable='box'` option to set 3D aspect ratio. * "What's New": `adjustable` argument of 3D plots aspect ratio.
Uh oh!
There was an error while loading. Please reload this page.
PR Summary
#23409 implemented equal aspect for 3D axes similar to the way
axes2d.set_aspect('equal', adjustable='datalim')works. This adds functionality similar to the wayaxes2d.set_aspect('equal', adjustable='box')works (i.e. without changing the limits on the axes of coordinates).The code is probably not the cleanest; style suggestions are appreciated.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).