-
Notifications
You must be signed in to change notification settings - Fork 1
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
remove use of deprecated np.float #20
Conversation
…still one can use date2num in any case.
Ska/Matplotlib/core.py
Outdated
@@ -186,7 +186,7 @@ def cxctime2plotdate(times): | |||
# Find the plotdate of first time and use a relative offset from there | |||
times = times.ravel() | |||
t0 = CxoTime(times[0]).unix | |||
plotdate0 = epoch2num(t0) | |||
plotdate0 = date2num(t0) * 1e6 |
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.
This really threw me for a loop. The MPL documentation is pretty bad since it implies that the argument t0
must be a np.datetime
type and makes no mention of what happens if you pass in a float. I ended up needing to read the source code to figure out what's happening.
So one thing is that the * 1e6
needs to go inside the date2num()
call, and adding a comment # Float arg to date2num is in microsecs
would have saved me 10 minutes of digging around.
This change should also be highlighted in the PR description.
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.
Yes, this is kind of a pain. Also, this function was deprecated only for a minor version, then they un-deprecated it :/
Anyway I figured it was ok to replace it. I will add comments.
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.
It has been re-deprecated since 3.5 from what I see in the latest stable docs. Strange.
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 just checked this and it was not working. I could swear I tested it, but it must have been with some intermediate version in which the date2num function was different.
In any case, the following line works both in our current version and on 3.5.0:
plotdate0 = date2num(CxoTime(times[0]).datetime)
I might check a couple intermediate versions to see.
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 not know what causes it, but now by chance I got to a configuration where the following statements are equivalent:
t = CxoTime()
epoch2num(t.unix)
date2num(t.datetime)
date2num(t.unix * 1e6)
I got this configuration without noticing and now I can retrace my steps (I basically tried three versions, it failed in all, then I decided to undo everything and then it worked, where it was failing before, so obviously something changed)
The bottomline is that we should take the second option, which worked in a fresh environment, and I suggest you double check.
The change to |
Description
This PR makes tiny changes to remove uses of a few deprecated dtypes (sot/skare3#753). This module has no tests, but the change is simple enough.
It also replaces
mpl.epoch2num
bympl.date2num
.mpl.epoch2num
is deprecated in 3.3.0 and un-deprecated in 3.3.1.Testing