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.

Sign up for GitHub

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

Jump to bottom

MovingMNIST split fix #7449

Merged
merged 7 commits into from
Mar 23, 2023
Merged

MovingMNIST split fix #7449

merged 7 commits into from
Mar 23, 2023

Conversation

Shu-Wan
Copy link
Contributor

@Shu-Wan Shu-Wan commented Mar 22, 2023 •
edited by pytorch-bot bot

Describe the fix

  1. torchvision/datasets/moving_mnist.py: Separate handling of split="test" condition
  2. test/test_datasets.py: Test split=None condition and verify that the second dimension equals _N_FRAMES

Check list

  • Code formatting
  • Unit test

Fix #7439

cc @pmeier

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 22, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7449

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot
Copy link

Hi @Shu-Wan!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@Shu-Wan Shu-Wan changed the title Shu wan/issue7439 MovingMNIST split fix Mar 22, 2023
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

pmeier
pmeier approved these changes Mar 23, 2023
View reviewed changes
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Shu-Wan! A few nits inline, but otherwise this is good to go if CI is green.

test/test_datasets.py Outdated Show resolved Hide resolved
test/test_datasets.py Outdated Show resolved Hide resolved
Shu-Wan and others added 2 commits March 23, 2023 01:42
@Shu-Wan @pmeier
Update test/test_datasets.py
bcaf0f1 
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
@Shu-Wan
1. change n to num for naming convention
7b53dc2 
2. revert format empty line changes
pmeier
pmeier reviewed Mar 23, 2023
View reviewed changes
test/test_datasets.py Outdated Show resolved Hide resolved
test/test_datasets.py Outdated Show resolved Hide resolved
@Shu-Wan
Copy link
Contributor Author

Shu-Wan commented Mar 23, 2023 •
edited

@pmeier, I believe all nits are fixed now. sorry for any confusion that I commented before I commit 👼🏼
lmk if there's any questions.

pmeier
pmeier approved these changes Mar 23, 2023
View reviewed changes
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

sorry for any confusion that I commented before I commit

No worries. Did that myself before.

LGTM if CI is green! Note that there is currently and outage that will be fixed by #7447. Thus, just because a CI job is red, doesn't mean your PR is at fault. I'll have a look and see if any failures are related.

@pmeier pmeier mentioned this pull request Mar 23, 2023
Closed
@pmeier pmeier merged commit dabb6d5 into pytorch:main Mar 23, 2023
pmeier added a commit to pmeier/vision that referenced this pull request Mar 23, 2023
@Shu-Wan @pmeier
[CHERRY-PICK] MovingMNIST split fix (pytorch#7449)
4bb4602 
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
pmeier added a commit that referenced this pull request Mar 23, 2023
@pmeier @Shu-Wan
[CHERRY-PICK] MovingMNIST split fix (#7449) (#7451)
2bda93b 
Co-authored-by: Shu <shu.wan@outlook.com>
@Shu-Wan Shu-Wan deleted the Shu-Wan/issue7439 branch March 23, 2023 22:22
facebook-github-bot pushed a commit that referenced this pull request Mar 30, 2023
@NicolasHug @facebook-github-bot
[fbsync] MovingMNIST split fix (#7449)
ad84aaa 
Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Reviewed By: vmoens

Differential Revision: D44416591

fbshipit-source-id: ff034766f5ede1718948fb90d5f40f10e0423683
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmeier pmeier pmeier approved these changes

Assignees
No one assigned
Labels
bug cla signed module: datasets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dataset MovingMNIST: split=None returns test dataset
3 participants
@Shu-Wan @facebook-github-bot @pmeier