Adding tests for timeseries module#219
Adding tests for timeseries module#219marty-larocque wants to merge 14 commits intoearthlab:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #219 +/- ##
==========================================
+ Coverage 79.95% 80.99% +1.04%
==========================================
Files 19 20 +1
Lines 1831 1863 +32
==========================================
+ Hits 1464 1509 +45
+ Misses 367 354 -13
Continue to review full report at Codecov.
|
…rrays when dtype was object
… when xy_data and xy_expected do not have the same shape.
| return TimeSeriesTester(axis) | ||
|
|
||
|
|
||
| """ |
There was a problem hiding this comment.
I will merge this for now but i think we should pull this documentation out into an issue somewhere rather than leaving it in the code.
lwasser
left a comment
lwasser
left a comment
There was a problem hiding this comment.
once ci rebuilds we can merge this. it essentially removes the xtime variable as we have discussed and thus avoids the issues associated with converting datetime objects.
|
@ryla5068 have you updated from master lately? i believe this pr is failing because it's missing some fixes from master. can you do an update and ping me when it's done and i'll watch CI again? |
lwasser
left a comment
lwasser
left a comment
There was a problem hiding this comment.
needs an update from master so CI passes
|
@nkorinek can you help me finish up this pr? let's go ahead and create a new branch off of this one and update from master to see if CI is happy. if not we can look into whether pillow needs to be in the dev requirements now |
|
this also needs a change log entry! |
nkorinek
left a comment
nkorinek
left a comment
There was a problem hiding this comment.
@ryla5068 just found a couple of small things that should be changed before this is merged, looks good!
| import matplotlib.pyplot as plt | ||
| import numpy as np | ||
| import pandas as pd | ||
|
|
There was a problem hiding this comment.
These import should be in pep8 order, matplotcheck being imported last
matplotcheck/base.py
Outdated
| except ValueError: | ||
| # xy_data and xy_expected do not have the same shape | ||
| raise AssertionError(message) |
There was a problem hiding this comment.
This should be
raise ValueError(
"xy_data and xy_expected do not have the same shape"
)
like the error raised above, right? Otherwise this except statement isn't very useful.
There was a problem hiding this comment.
let's go ahead and raise a valueError here!!
Co-Authored-By: Nathan Korinek <nako1890@colorado.edu>
|
@ryla5068 let's try to wrap up this pr this week. nathan left a few comments and it looks like there is a linting issue. once those are addressed i can merge this |
|
@lwasser This should be ready to be merged. |
| try: | ||
| np.testing.assert_array_max_ulp( | ||
| np.array(xy_data["y"]), np.array(xy_expected[ycol]) | ||
| np.array(xy_data["y"]), np.array(xy_expected[ycol]), 5 |
There was a problem hiding this comment.
curious what the 5 is here that was added? @nkorinek when you look at this PR can you please see if it makes sense to you to have 5 there?
|
ok @nkorinek the changes needed here are minor. i have one question about a test that has the number 5 in it -- that was added in this pr and i want to ensure that should be there. and then we just need to sync up the change log as the md and the rst version are now in this pr.
|
|
@lwasser ok, so the 5 was added into the I don't see any reason for this number to be changed. I took out the argument all together and all of the tests still pass. I'll open up a new pr with those number removed and see if anything breaks, but I don't see a reason this was added in, unless I'm missing something. |
|
closed!. thanks @nkorinek !! |
3 participants
This PR adds tests for the timeseries module, addressing issue #186. It also changes the method for using
assert_xy()with datetime data, addressing issue #169 .Additionally, it implements some minor tweaks to how assert_xy() compares data, addressing issues #232 and #233.