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

Fix transform call in ImageFolderDataset #15672

Open
wants to merge 4 commits into
base: master
from

Conversation

@john-
Copy link

commented Jul 27, 2019

Description

Without this change I get the following (partial) error:

Expected exactly one or two argument for an accessor of transform at
/home/user/perl5/lib/perl5/AI/MXNet/Gluon/Data/Vision.pm line 422, line 207.

This change addresses that error.

Checklist

Essentials

I will take a look at how to add unit test to this change.

Changes

There are no new features. From my perspective the change addresses a typo as other packages in the same file call the transform sub the same way this change does.

Comments

I could not get original code to work so cannot confirm if change is backwards compatible.

@john- john- requested a review from sergeykolychev as a code owner Jul 27, 2019

@sergeykolychev
Copy link
Contributor

left a comment

@john- Thank you for the fix, I'll merge it next week.

@sergeykolychev

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@john- the tests seem to be stuck, can you please merge master to you branch and push to trigger new set of tests ? Thanks!

@john-

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

I am not quite sure what I am doing but so far I have:

  • Did a pull of master from apache/incubator-mxnet
  • Merge master into my branch
  • Pushed the branch back to my repo

When you say push to trigger a new set of tests does that mean do another pull request?

@sergeykolychev

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@john- thank you, I'll try to get the tests running

@sergeykolychev

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@szha Hi there, could you check when you have time why tests are stuck on this pr ?

@piyushghai

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@mxnet-label-bot Add [Perl, Gluon]

@ChaiBapchya

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Thanks for the contribution @john-
I guess, way around for this would be to retrigger CI using
git commit --allow-empty -m "Trigger notification"
Hopefully this time it doesn't get stuck.

@roywei

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@john- gentle ping to trigger CI. thanks!

@john-

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

Do I need to:

  1. Close this pull request
  2. Do a 'git commit --allow-empty -m "Trigger notification"' in my branch
  3. Do another pull request

I don't see how doing the 'git commit --allow-empty -m "Trigger notification"' will do anything otherwise. Also, note that I merged master back into my branch.

@john-

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

I did the trigger notification commit and pushed it to my branch. I see it listed as a commit in the pull request. Should I do something else to make this work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.