earthlab / matplotcheck

A python package for checking and testing matplotlib plots. We use this for autograding student assignments but there are many other potential use cases including package testing (for packages with plots)!

https://matplotcheck.readthedocs.io

BSD 3-Clause "New" or "Revised" License

18 stars 8 forks source link

Adding tests for timeseries module #219

Closed marty-larocque closed 4 years ago

marty-larocque commented 4 years ago

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.

codecov[bot] commented 4 years ago

Codecov Report

Merging #219 into master will increase coverage by 1.04%. The diff coverage is 90.24%.

@@            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

Impacted Files	Coverage Δ
matplotcheck/base.py	`87.66% <50.00%> (-0.05%)`	:arrow_down:
matplotcheck/tests/test_timeseries_module.py	`100.00% <100.00%> (ø)`
matplotcheck/timeseries.py	`21.66% <0.00%> (+21.66%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eab6e72...c3990da. Read the comment docs.

lwasser commented 4 years ago

@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 commented 4 years ago

@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

lwasser commented 4 years ago

this also needs a change log entry!

lwasser commented 4 years ago

@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

marty-larocque commented 4 years ago

@lwasser This should be ready to be merged.

lwasser commented 4 years ago

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. so please

make sure the .rst file has the most recent changes - marty i think adds them at the bottom of the list so just make sure all unreleased stuff is captures and then
delete the md file from this pr! then we can merge.

nkorinek commented 4 years ago

@lwasser ok, so the 5 was added into the assert_array_max_ulp() for the maxulp argument, which is described as:

maxulp: int, optional
    The maximum number of units in the last place that elements of a and b can differ. Default is 1

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.

nkorinek commented 4 years ago

@lwasser this can be closed due to #273

lwasser commented 4 years ago

closed!. thanks @nkorinek !!