Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix transform call in ImageFolderDataset #15672

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

john-
Copy link

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

Copy link
Contributor

@sergeykolychev sergeykolychev left a comment

Choose a reason for hiding this comment

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

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

@sergeykolychev
Copy link
Contributor

@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-
Copy link
Author

john- 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
Copy link
Contributor

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

@sergeykolychev
Copy link
Contributor

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

@piyushghai
Copy link
Contributor

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

@ChaiBapchya
Copy link
Contributor

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
Copy link
Member

roywei commented Aug 19, 2019

@john- gentle ping to trigger CI. thanks!

@john-
Copy link
Author

john- 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-
Copy link
Author

john- 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?

@john-
Copy link
Author

john- commented Oct 12, 2019

I read through the Details for the ci failures and it is not clear to me how the errors relate to the pull request. Can someone provide some insight? Maybe I need to re-merge the master branch?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants