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

Reduce tests for video, fix video load, & remove binder icon #178

Merged
merged 9 commits into from Jul 6, 2021

Conversation

xianyuanliu
Copy link
Member

@xianyuanliu xianyuanliu commented Jul 5, 2021

Fixes #175.

Description

  • Reduce tests for video
  • Fix video load
  • Remove binder icon

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • In-line docstrings updated.
  • Source for documentation at docs manually updated for new API.

@xianyuanliu xianyuanliu added the work-in-progress Work in progress that should NOT be merged label Jul 5, 2021
@github-actions github-actions bot added this to In progress in v0.1.0 Jul 5, 2021
@haipinglu
Copy link
Member

@bobturneruk Forcing pandas>=1.3.0 is not working for 3.6
https://github.com/pykale/pykale/runs/2993309532
ERROR: Could not find a version that satisfies the requirement pandas>=1.3.0

@xianyuanliu You did not seem to have fixed pandas 1.3.0 compatibility problem yet.

@xianyuanliu
Copy link
Member Author

xianyuanliu commented Jul 5, 2021

Yes, I haven't solved it yet because it seems a general problem. I report it to pandas.
Dataframes saved with pandas<1.3.0 and "pickle" is no longer loadable with pandas 1.3.0 in pandas-dev/pandas#42345.

Btw, Pandas added this problem to the 1.3.1 milestone which due time is July 25.

@haipinglu
Copy link
Member

Yes, I haven't solved it yet because it seems a general problem. I report it to pandas.
Dataframes saved with pandas<1.3.0 and "pickle" is no longer loadable with pandas 1.3.0 in pandas-dev/pandas#42345.

Btw, Pandas added this problem to the 1.3.1 milestone which due time is July 25.

Good finding! test_video_feature_extractor has several hundreds (?) tests, 60+% of all tests. Can they be reduced as well?

@mustafa1728
Copy link
Contributor

mustafa1728 commented Jul 6, 2021

I tried using the pandas function pd.read_pickle() instead of pickle.load(), and the tests pass on my local.

# with open(self.annotationfile_path, "rb") as input_file:
#     input_file = pickle.load(input_file)
 input_file = pd.read_pickle(self.annotationfile_path)

May I test on GitHub by committing in this PR?

It seems that pandas is handling this exception in pd.read_pickle() (source), as mentioned in this comment.

@haipinglu
Copy link
Member

May I test on GitHub by committing in this PR?

Yes, please go ahead.

@xianyuanliu
Copy link
Member Author

xianyuanliu commented Jul 6, 2021

I have tested pickle.load and pd.read_pickle by the code below in the previous Pandas version.

with open(path, "rb") as input_file:
    input_file1 = pickle.load(input_file)
with open(path, "rb") as input_file:
    input_file2 = pd.read_pickle(input_file)
print(pd.testing.assert_frame_equal(input_file1, input_file2))

Output is None, which means no difference between input_file1 and input_file2.

Like what @mustafa1728 said, pandas uses pickle.load inside pd.read_pickle but updated this function this time. But I haven't tested whether they are the same in the newest version.

@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2021

Codecov Report

Merging #178 (825d2d0) into main (04b2863) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   88.37%   88.36%   -0.01%     
==========================================
  Files          45       45              
  Lines        4197     4195       -2     
==========================================
- Hits         3709     3707       -2     
  Misses        488      488              
Impacted Files Coverage Δ
kale/loaddata/video_datasets.py 86.44% <100.00%> (-0.45%) ⬇️

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 04b2863...825d2d0. Read the comment docs.

@xianyuanliu
Copy link
Member Author

@mustafa1728 Do we need this with ... open ... here to avoid forgetting to close the file? I am not sure if it is needed.

https://stackoverflow.com/questions/20101021/how-to-close-the-file-after-pickle-load-in-python

@mustafa1728
Copy link
Contributor

mustafa1728 commented Jul 6, 2021

Do we need this with ... open ... here to avoid forgetting to close the file? I am not sure if it is needed.

The read_pickle() function has a with ... open ... block inside it here, so its not needed, as it is closing the file anyways. However, this works both ways (path or file handler as in the docs), so it should not cause any errors inside the with ... open block either.

@xianyuanliu
Copy link
Member Author

xianyuanliu commented Jul 6, 2021

Oh, my mistake. I regarded it as pickle.load, but not the new one. Thanks.

@xianyuanliu xianyuanliu requested review from haipinglu and removed request for haipinglu July 6, 2021 18:31
@xianyuanliu xianyuanliu added tests Tests and coverage and removed work-in-progress Work in progress that should NOT be merged labels Jul 6, 2021
@haipinglu haipinglu changed the title Reduce test_video_access Reduce tests for video and fix video load Jul 6, 2021
@haipinglu haipinglu changed the title Reduce tests for video and fix video load Reduce tests for video, fix video load, & remove binder icon Jul 6, 2021
@haipinglu haipinglu enabled auto-merge July 6, 2021 18:46
@haipinglu haipinglu merged commit 7b47194 into main Jul 6, 2021
v0.1.0 automation moved this from In progress to Done Jul 6, 2021
@haipinglu haipinglu deleted the reduce_video_access_test branch July 6, 2021 19:25
@haipinglu haipinglu linked an issue Jul 6, 2021 that may be closed by this pull request
@github-actions github-actions bot mentioned this pull request Sep 10, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests and coverage
Projects
No open projects
v0.1.0
  
Done
4 participants