Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Defined the test_clabel function in the test_datetime file#27490
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I think the test here is that the contour x and/or y axes are datetimes, and that the manual placement of clabels can be specified with datetimes. I'm actually skeptical that this works, but if not, would be good to document it. |
Ali-Aboelela commentedDec 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
How would I be able to test this? I've tried a few different things but only ran into errors. Edit: Perhaps this PR should be modified to just delete the test? I don't think what#26864 is attempting to test makes sense for clabel. |
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. 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.
jklymak commentedDec 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@AliAboelela123 I've not looked in detail, but it is possible to contour with https://matplotlib.org/stable/api/contour_api.html#matplotlib.contour.ContourLabeler.clabel |
rcomer commentedDec 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
One use-case for z datetimes is to plot amap of monsoon onset, though I agree that having datetimes on the x- or y-axis (e.g.Hovmöller diagrams) is more common. Perhaps it is worth testing both? |
If the errors show that we don't currently support this, then that is useful information and part of what the issue is looking to find out. So please post your findings 👍 |
I've made some updates, I don't expect this to be merged soon but rather just trying to continue the conversation. I summarized my findings in an edit |
@AliAboelela123 I still think you are misunderstanding. The x array should be datetimebefore sending to contour. Subsequently clabel should be called with the manual kwarg. |
The z data in contour is not run through unit machinery, so all we would be testing is that clabel takes an arbitrary string. |
…etime objects directly to contour when creating contour plot using clabel
Ok I've tried to fix the PR by constructing the contour using datetime objects and passing that as input to the clabel function. I have not used the manual kwarg though. Do the current changes more accurately reflect what this test suite is attempting to test? |
No? What are you not understanding about the "manual" kwarg? |
Ali-Aboelela commentedDec 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I tried to pass the above ^^ "manual" param to the clabel function but I get the following error: "TypeError: Cannot cast array data from dtype('O') to dtype('float64') according to the rule 'safe'" This makes me think that the manual param expects the tuples to have float type i.e. coordinates in a plane rather than data type 'O' which I think means object and is referencing the datetime obj. I'm not sure if I'm doing something wrong or if this is the intended limitation of the clabel function which we are attempting to document. |
I didn't test it myself, but if the manual clabel doesn't work then that would be a bug report issue. Probably we should process the unit info for the clabel manual inputs. This PR could still test that labelling works when the x and y axis are datetimes. Somewhat more trivial, but worth checking. |
I've opened#27525 for the |
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 don't think the code as it is written here is exercising the datetimes at all really, it is only using them as labels (passed throughstr
, not processed through our unit system)
I think that themanual
behavior described in#27525 would be a good thing to test here, but that doesn't yet work
As for datetimes on the Z axis, we simply don't have any native support for that at this time, and that would be a larger change.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
This PR is one of the issues identified by the larger issue#26864. These changes add a test for the clabel function. I have tested passing datetimes as input labels to the contours and the axis.
I could not think of a way nor do I think it to make sense to pass datetime objects as labels to either axis.Edit: Attempt 3
This time I correctly generate the contour using datetime objects and directly pass it to the clabel function. Passing a contour that was generated where either X or Y values contain datetime objects functions correctly. I simply avoided doing any math related to the inputs to find Z. TheHovomoller diagram@rcomer mentioned helped me visualize how this kind of contour plot with dates would be used which helped me form an example.
Note: I have NOT been able to specify the locations of the labels using the manual kwarg using datetime objects, please see discussion/comments below
Note: I did not use the example where x-axis was used as a date (commented in the code) because it was quite messy, instead I've provided the plot generated when datetime objects are used to set the y-axis of a contour plot.
Edit: Attempt 2
Original PR summary (Attempt 1)
Here is the image generated by this code:
PR checklist
[N/A] "closes #0000" is in the body of the PR description tolink the related issue
new and changed code istested
I have struggled to run pytest due to a circular import issue. Instead, the image of the plot was generated using a separate script.
[N/A]Plotting related features are demonstrated in anexample
[N/A]New Features andAPI Changes are noted with adirective and release note
[N/A] Documentation complies withgeneral anddocstring guidelines