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

Date fixes #3947

Merged
merged 23 commits into from Jul 22, 2015
Merged

Date fixes #3947

merged 23 commits into from Jul 22, 2015

Conversation

pganssle
Copy link
Member

Here are some fixes to some of the problems discussed in Issue #2845. Sorry I worked off of the master branch.

I consolidated all the "magic numbers" into a central location so they are consistent and easy to update if necessary, and I updated the to_ordinalf and from_ordinalf functions to use datetime.timedelta rather than deal with converting fractional days to datetime objects and vice versa.

…triple-quote docstrings for consistency.)

Signed-off-by: Paul G <p.ganssle@gmail.com>
Signed-off-by: Paul G <p.ganssle@gmail.com>
…or the moment.

Signed-off-by: Paul G <p.ganssle@gmail.com>
…om `datetime`.

Signed-off-by: Paul G <p.ganssle@gmail.com>
…nversion.

Signed-off-by: Paul G <p.ganssle@gmail.com>
@pganssle
Copy link
Member Author

One thing to note is that in the current master branch, dates.py is inconsistent between how long a "month" is. In some places it's 31 days, in other places 30 days. By switching all these references to DAYS_PER_MONTH, I set them all to 30. It seems minor, but that seems like the main thing that might change behavior.

I'll have to look through what the results would be, but I'm wondering if it might be a good idea to set DAYS_PER_MONTH to be DAYS_PER_YEAR/MONTHS_PER_YEAR, which would be 365.0 / 12.0 or 30.41666. I'm not sure what the consequences of that would be. Similarly, since I've changed all references to the number of days per year from 365.0 to DAYS_PER_YEAR, I'm wondering if it would be more accurate to set DAYS_PER_YEAR to some number closer to an actual year, like 365.242.

Signed-off-by: Paul G <p.ganssle@gmail.com>
…ects.

Signed-off-by: Paul G <p.ganssle@gmail.com>
Signed-off-by: Paul G <p.ganssle@gmail.com>
@pganssle
Copy link
Member Author

Sorry these builds keep failing - I don't have my full normal set up at the moment and its a bit of a pain to set up the Windows builds, so I can't do much more than basic unit tests. I also don't have my normal linting set up. I'll try and get everything sorted soon and make sure it passes the build tests.

@tacaswell
Copy link
Member

Thanks for working on this.

I think master branch is the right place for these changes.

It looks like at least part of the problem is that the converted times are now coming out with a timezone attached to them.

Signed-off-by: Paul G <p.ganssle@gmail.com>
Signed-off-by: Paul G <p.ganssle@gmail.com>
@jbmohler
Copy link
Contributor

@pganssle It shouldn't be a pain to build on windows using https://github.com/jbmohler/matplotlib-winbuild . I'm paying attention to any gotcha's on that.

@pganssle
Copy link
Member Author

@jbmohler Thanks for the info. I saw that, but I don't have Visual Studio installed on this computer and it seems I have to register with Microsoft to get it, which I'm not really interested in doing. I'll either bite the bullet and register or I'll set up a virtual linux container here, because at the moment I've got something rigged up with cygwin that seems to just be limping along.

…till fail, but that's fine for now.

Signed-off-by: Paul G <p.ganssle@gmail.com>
@jbmohler
Copy link
Contributor

@pganssle I do not believe that you need to register for the express edition of 2008 (Python 2.7) or 2010 (Python 3.3/4). I think you cannot build 64 bit with the express edition.

…ys between two points.

Signed-off-by: Paul G <p.ganssle@gmail.com>
@pganssle
Copy link
Member Author

OK, so I've cleared up most of the problems with the build failing. One that still remains arises from the fact that I've actually changed the behavior of AutoDateLocator.get_locator(), which was actually potentially incorrectly calculating the number of days between two events, because it needed to make an estimate of the number of days in a year and the number of months in a year. This will cause tests.test_dates.test_auto_locator() to fail, since that asserts that the date location logic hasn't changed (which it has, slightly). I'm thinking that the solution to this is to just change the values of the assert, since the expected behavior is changing, but I didn't want to presume to do that myself.

I'm not 100% clear on why tests.test_dates.test_DateFormatter() is failing.

This is the expected result:

~~And this is the actual result:~~

~~I'm guessing something I've done changes the direction or sign of some rounding errors, which cascades into this, but it doesn't seem like a _problem_, per se. I'll try and figure out exactly what's causing it so we can decide whether this new behavior is the result of something that might have other consequences. My guess is there won't be any significant consequences, but maybe while tracking it down I can figure out why the auto locator isn't choosing round numbers for fractional seconds in the first place (i.e. 0.2, 0.4, 0.6, 0.8), as that seems like it would be a preferable behavior anyway.~~ **Edit** Nevermind - seems to just be a difference between my build environment and the standard one, since that test is no longer failing on the Travis CI builds.

…lta object.

Signed-off-by: Paul G <p.ganssle@gmail.com>
pganssle and others added 6 commits December 26, 2014 19:58
…timedelta` object actually calculates those for us as is.

Signed-off-by: Paul G <p.ganssle@gmail.com>
…e date locator.

Signed-off-by: Paul G <p.ganssle@gmail.com>
…vious versions.

Signed-off-by: Paul G <pg@example.com>
Signed-off-by: Paul G <pg@example.com>
Signed-off-by: Paul G <pg@example.com>
@pganssle
Copy link
Member Author

@tacaswell Good call. Was thinking of it as an instance method, but it makes more sense to think of it as an alias to the static call anyway.

…hen dateutil <= 2.3

Signed-off-by: Paul G <pg@example.com>
…en necessary.

Signed-off-by: Paul G <pg@example.com>
@pganssle
Copy link
Member Author

By the way, I know I mentioned on the dev list that I was going to look into using numpy's datetime64, but I haven't had time yet, and these date fixes are pretty much ready to go independent of that.

Please let me know if it needs to be rebased or if you want me to squash some of these commits or whatever, or anything else I need to fix, I suppose.

@tacaswell tacaswell added this to the v1.5.x milestone Jan 30, 2015
@tacaswell
Copy link
Member

Thanks for the ping, this had fallen off my radar.

@mdboom How is the expert dates?

'1990-02-17 00:00:00+00:00', '1990-03-10 00:00:00+00:00',
'1990-03-31 00:00:00+00:00', '1990-04-21 00:00:00+00:00',
'1990-05-12 00:00:00+00:00']
[datetime.timedelta(days=141),
Copy link
Member

Choose a reason for hiding this comment

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

I remember talking about this before, but not the conclusion. Why is it OK that the tests were changed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tacaswell It's an expected (minor) behavior change caused by an increase in the accuracy of the way marker locations are calculated. I think the change was in this commit; previously if the beginning and end dates were January 3rd, 2000 and June 10th, 2015, relativedelta object returns (years=15, months=5, days=7), and when get_locator() is called, it calculated the number of days between the dates assuming that the number of days per year is 365 and the number of days per month is 30, so the number of days calculated would be 5632 rather than the true value of 5637.

It's a small inaccuracy, but it's easy to fix by holding on to a timedelta object and using that when calculating days from years or months. As a result, some of the date range calculations are slightly different (though not to a degree that anyone would really notice), so at least one of the tests had to be updated (I vaguely remember updating one that was around 3 months and one that was around 3 weeks).

(Oops, responded to this in the PR not the line)

@tacaswell tacaswell modified the milestones: 1.5.0, v1.5.x Feb 7, 2015

Currently `datetime.datetime.strftime()` is not supported for
years <= 1900 in Python 2.x and years <= 1000 in Python 3.x. This
function extends this functionality to
Copy link
Member

Choose a reason for hiding this comment

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

This cuts off rather abruptly

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I'll fix this a bit later this week.

Copy link
Member

Choose a reason for hiding this comment

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

Does not matter, removed in favor of #3242's changes

@tacaswell
Copy link
Member

@pganssle Two super minor comments. This look almost ready!

@WeatherGod
Copy link
Member

@pganssle, this needs a rebase, and address @tacaswell 's two minor comments.

@tacaswell
Copy link
Member

This also needs a rebase.

@pganssle
Copy link
Member Author

Can do. Just started a new job, so I had to sort moving and IP issues. No
problems, so I should be able to get to it this weekend.
On Jul 16, 2015 12:18 AM, "Thomas A Caswell" notifications@github.com
wrote:

This also needs a rebase.


Reply to this email directly or view it on GitHub
#3947 (comment)
.

@tacaswell
Copy link
Member

I know how that goes, congratulations!

On Thu, Jul 16, 2015, 6:46 AM Paul Ganssle notifications@github.com wrote:

Can do. Just started a new job, so I had to sort moving and IP issues. No
problems, so I should be able to get to it this weekend.
On Jul 16, 2015 12:18 AM, "Thomas A Caswell" notifications@github.com
wrote:

This also needs a rebase.


Reply to this email directly or view it on GitHub
<
https://github.com/matplotlib/matplotlib/pull/3947#issuecomment-121825037>
.


Reply to this email directly or view it on GitHub
#3947 (comment)
.

@tacaswell
Copy link
Member

This has major conflicts with the changes merged in #3242

@tacaswell tacaswell modified the milestones: proposed next point release, next point release Jul 22, 2015
@pganssle
Copy link
Member Author

Yes, I noticed this when trying to rebase this past weekend. Working on resolving the conflicts.

@tacaswell tacaswell merged commit ad55256 into matplotlib:master Jul 22, 2015
tacaswell added a commit that referenced this pull request Jul 22, 2015
Merged locally to deal with conflicts arising from #3242 (merged as
8609886) which also touched the
pre-1900 date formatting code.

Resolved conflicts in favor of #3242 which refactored the pre-1900 code
into a helper function.
@tacaswell
Copy link
Member

@pganssle Ah, sorry didn't realize you were working on this! I just did the merge/conflict resolution and all the tests pass locally....

@pganssle
Copy link
Member Author

@tacaswell Oh, no problem if it works. I had done the rebase but couldn't get the tests to run on my laptop (Windows), so I was just trying to transfer it over to a linux virtual box to test it. If the tests pass then I'm fine with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants