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 test_axhline in test_datetime.py #27177

Merged
merged 4 commits into from
Oct 26, 2023
Merged

Conversation

CozyFrog
Copy link
Contributor

PR summary

I have added a datetime (no timedelta; this function does not support it, and I believe that is correct) smoke test for Axes.axhline in lib/matplotlib/test/test_datetime.py.
This addresses the Axes.axhline task from #26864.

The image below is the plot generated from my example code.
test_axhline

PR checklist

Comment on lines 34 to 44
birth_date = np.array([datetime.datetime(2020, 4, 10),
datetime.datetime(2020, 5, 30),
datetime.datetime(2020, 10, 12),
datetime.datetime(2020, 11, 15)])
year_start = datetime.datetime(2020, 1, 1)
year_end = datetime.datetime(2020, 12, 31)
age = [21, 53, 20, 24]
ax.set_xlabel('Birth Date')
ax.set_ylabel('Age')
ax.set_ylim(bottom=year_start, top=year_end)
ax.bar(x=age, height=birth_date)
Copy link
Member

Choose a reason for hiding this comment

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

I would say to keep this even more minimal and not have the bar graph (I think setting the ylim is important here)

If anything, I'd just do a ax.plot, but even that I think is not testing what we are looking to test here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good, thank you for the feedback!

@CozyFrog
Copy link
Contributor Author

I simplified the example and added xmin and xmax arguments.
axhline_simplified

Copy link
Contributor

@scottshambaugh scottshambaugh left a comment

Choose a reason for hiding this comment

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

Could you add subplots to test different combinations of numbers and datetimes on different axes? Similar to the test you added here: #27084

@CozyFrog
Copy link
Contributor Author

Sure thing. Can you elaborate on what you mean by different axes? I can test axhline on different plots, but I am unsure how to utilize it on different axes as it itself only plots on the horizontal. Below is an image of what I have come up with so far. Thank you for your feedback!
axhline_3figures

@scottshambaugh
Copy link
Contributor

scottshambaugh commented Oct 24, 2023

@CozyFrog ah I mean having subplots where the X/Y data are different combinations of datetime vs numbers. Right now you have datetimes on the Y axis with numbers on the X axis, and adding the other permutations of that should exercise these codepaths more fully (I think). Your other PR did this well.

@ksunden
Copy link
Member

ksunden commented Oct 24, 2023

@scottshambaugh I don't think that actually makes sense for this method, honestly... the x axis values are in axes coordinates, not data coordinates (well, unless you hand a transform argument), so really the only value handed to axhline that matters for units is the y value.

@scottshambaugh
Copy link
Contributor

I'll defer to you on that @ksunden, I'm admittedly not super familiar with how the code paths for the coordinate transforms work work. If it's drawn directly in axes coordinates they I think you're right, my thought was if the xrange was cast to the data coordinates before being drawn.

@CozyFrog
Copy link
Contributor Author

Thank you for the discussion @scottshambaugh & @ksunden – how should I move forward with this PR? Does this require any modifications?

@ksunden
Copy link
Member

ksunden commented Oct 26, 2023

Yes, the X values are drawn directly in axes coordinates, and in fact the method explicitly checks that those values do not have units and prevents passing an alternate transform even.

Thus I think we should be good to go here, though we do have a 2-reviewer policy for non-doc changes (And I was the first approver, soo).

@ksunden ksunden added this to the v3.9.0 milestone Oct 26, 2023
@QuLogic QuLogic merged commit 775fbc0 into matplotlib:main Oct 26, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants