Movatterモバイル変換

Copy link

github-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

Copy link

Member

jklymak commentedDec 7, 2022

Thanks for the PR! For ease of review, suggest a description of the change and moving the issues from title to the description.

oscargus reviewed

lib/matplotlib/offsetbox.py Outdated

		@@ -369,12 +369,12 @@ def draw(self, renderer):


		class PackerBase(OffsetBox):
		def __init__(self, pad=None, sep=None, width=None, height=None,
		def __init__(self, pad=0., sep=None, width=None, height=None,

Copy link

Member

oscargusDec 7, 2022•
edited
Loading

Choose a reason for hiding this comment

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

I think the same problem holds for sep as well?

Edited: width and height are calculated, but sep must be set.

oscargus reviewed

lib/matplotlib/offsetbox.py Outdated

		align="baseline", mode="fixed", children=None):
		"""
		Parameters
		----------
		pad : float,optional
		pad : float,required

Copy link

Member

oscargusDec 7, 2022

Choose a reason for hiding this comment

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

It is still optional, just that it has a correct default now.

Copy link

Member

oscargus commentedDec 7, 2022

Can you please how the outcome of the example code in#24620? Ideally as an image test, but if nothing else just paste the result here.

oscargus reviewed

lib/matplotlib/tests/test_offsetbox.py Outdated

		try:
		pb = PaddedBox(at, patch_attrs={'facecolor': 'r'}, draw_frame=True)
		except:
		raise Exception("incorrect default value")

Copy link

Member

oscargusDec 7, 2022

Choose a reason for hiding this comment

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

There is no need to do this in a test. The test will fail if there is an exception anyway, so skip the try-except.

You can add a comment like

# smoke test for correct default value though

Skhaki18 added2 commits

December 7, 2022 03:30

Update offsetbox.py with requested doc change and default sep

895c861

Added requested change for test_paddedbox

5b3397e

Copy link

Contributor

Skhaki18 commentedDec 7, 2022

This was our result when running the code from24620 with the default padding of 0. Additionally, we can see the result for padding=5 below:

jklymak changed the title~~[#24623][#24620] Offsetbox arguments and incorrect location~~Offsetbox arguments and incorrect location

jklymak added status: needs tests status: needs clarificationIssues that need more information to resolve. labels

Copy link

Member

jklymak commentedDec 7, 2022

I'm moving this to draft. Feel free to move back when ready for review.

jklymak marked this pull request as draft

December 7, 2022 16:46

Copy link

Member

oscargus commentedDec 8, 2022

There is an update in#24620 which shows that PaddedBox is indeed working. And it looks like the proposed solution is not fully handling it.

Hence, my suggestion is to drop the location changes and just go with the default argument ones.

updated to default values

8b8b539

Copy link

Contributor

Krutarth-P commentedDec 9, 2022

There is an update in#24620 which shows that PaddedBox is indeed working. And it looks like the proposed solution is not fully handling it.
Hence, my suggestion is to drop the location changes and just go with the default argument ones.

We have made the changes according to your request to use the default values. Would you know how to resolve the CircleCi errors?

oscargus reviewed

Dec 9, 2022

lib/matplotlib/offsetbox.pyShow resolvedHide resolved

Copy link

Member

oscargus commentedDec 9, 2022

The Circle CI issue is a bit weird. It it not related to the contents of your PR anyway. I think I've seen some issue recently where a particular user couldn't run the tests and the best guess was that the GitHub user name was changed. Would that possibly be the case here? Has anyone of you changed GitHub user name after you created your account?

Copy link

Contributor

Krutarth-P commentedDec 10, 2022

The Circle CI issue is a bit weird. It it not related to the contents of your PR anyway. I think I've seen some issue recently where a particular user couldn't run the tests and the best guess was that the GitHub user name was changed. Would that possibly be the case here? Has anyone of you changed GitHub user name after you created your account?

change of name doesn't seem to be the case here. any other possible reasons for the error?

Copy link

Contributor

Krutarth-P commentedDec 11, 2022

@oscargus @jklymak the builds are successful. We are ready for a review

rcomer marked this pull request as ready for review

December 11, 2022 08:18

oscargus reviewed

Dec 12, 2022

lib/matplotlib/tests/test_offsetbox.py

Copy link

Member

oscargus left a comment

Choose a reason for hiding this comment

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

Just some minor comments. The testing is good to get correct though.

One may consider changingoptional todefault: 0.0 in the doc strings. (Or addDefault: 0.0. to the description, but I think the first is preferred.)

		# smoke test for correct default value though
		fig, ax = plt.subplots()
		at = AnchoredText("foo", 'upper left')
		pb = PaddedBox(at, patch_attrs={'facecolor': 'r'}, draw_frame=True)

Copy link

Member

oscargusDec 12, 2022

Choose a reason for hiding this comment

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

This test passes on main as well. (As the fact that pad is None is not a problem until later on.)

You need to do:

ax.add_artist(pb)fig.draw_without_rendering()

If you can also add a similar thing for one of the packers that would be great! Can go in the same test/figure. There is a previous test for the packers that you can just copy the relevant code from, but leave the pad and sep arguments out.

lib/matplotlib/tests/test_offsetbox.py OutdatedShow resolvedHide resolved

lib/matplotlib/offsetbox.py OutdatedShow resolvedHide resolved

Krutarth-Pand others added5 commits

December 12, 2022 14:33

Update lib/matplotlib/offsetbox.py

a1ba0cd

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

Update lib/matplotlib/tests/test_offsetbox.py

ca9bc4a

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

Update lib/matplotlib/offsetbox.py

0f1f182

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

updated doc strings

4d1272c

added smoke test for packer

c8c1f87

Krutarth-Pand others added4 commits

December 12, 2022 15:01

Update test_offsetbox.py

f09389d

Update test_offsetbox.py

ad9fc5f

Update test_offsetbox.py

8009e09

Update test_offsetbox.py

bfbb4b0

Copy link

Contributor

Skhaki18 commentedDec 14, 2022•
edited
Loading

@oscargus and@rcomer , We have addressed the requested comments and added the necessary tests. We are able to build successfully and are ready for the next step in merging.

oscargus approved these changes

Copy link

Member

oscargus left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Thanks!

I guess this is good to go now. The addition of HPacker/VPacker tests is maybe not so clear, but it will work and test what is expected.

An better option is to simply remove the two previous lines where pad and sep are set as they are now redundant.

Whoever merges this: please squash the commits. (I take it that you are using the web-based interface for editing the files?)

oscargus removed status: needs clarification

Issues that need more information to resolve.

status: needs tests labels

oscargus changed the title~~Offsetbox arguments and incorrect location~~Offsetbox default arguments

timhoffm reviewed

lib/matplotlib/tests/test_offsetbox.py OutdatedShow resolvedHide resolved

Update lib/matplotlib/tests/test_offsetbox.py

cd6ecaf

Copy link

Member

timhoffm commentedDec 14, 2022

An better option is to simply remove the two previous lines where pad and sep are set as they are now redundant.
r editing the files?)

I took the liberty to commit this change.

timhoffm approved these changes

timhoffm added this to thev3.7.0 milestone

timhoffm added the Maintenance label

timhoffm merged commit58de4e2 intomatplotlib:main