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

adds get_fixed_examples to TemplateIterator #268

Merged
merged 11 commits into from
Jul 17, 2020

Conversation

theRealSuperMario
Copy link
Collaborator

No description provided.

@theRealSuperMario theRealSuperMario requested a review from pesser May 12, 2020 10:13
Copy link
Owner

@pesser pesser left a comment

Choose a reason for hiding this comment

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

This will be a great feature, but

  • it would be great if one can specify which dataset to take the examples from, eg train or validation.
  • I think the interface would be more consistent if the function just returns a batch exactly the way the step_op receives the batch instead of having to specify the keys. In addition it would be great if we can make sure that the behavior is exactly that of the dataloader - I think this is readily achieved by using iterators.batches.deep_lod2dol

)
fixed_examples = {}
for n in names:
fixed_examples_n = [self.dataset[i][n] for i in fixed_example_indices]
Copy link
Owner

Choose a reason for hiding this comment

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

this is unnecessarily inefficient. Make sure get_example is called only once for each index, eg fixed_examples = [self.dataset[i] for i in fixed_example_indices] outside of the for n in names loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

"stickman": batch of self.dset[i]["stickman"] with fixed i
}

TODO: document usage somewhere
Copy link
Owner

Choose a reason for hiding this comment

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

my best guess: this will never happen once that todo is committed. I suggest doing the todos immediately or removing the todo comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@theRealSuperMario
Copy link
Collaborator Author

  • I think the interface would be more consistent if the function just returns a batch exactly the way the step_op receives the batch instead of having to specify the keys. In addition it would be great if we can make sure that the behavior is exactly that of the dataloader - I think this is readily achieved by using iterators.batches.deep_lod2dol

Done

@theRealSuperMario
Copy link
Collaborator Author

it would be great if one can specify which dataset to take the examples from, eg train or validation.

Done

@pesser
Copy link
Owner

pesser commented Jul 17, 2020

Great! Now I can finally get rid of the copy & pasted code in my iterators :) 👍

@pesser pesser merged commit 3fab27c into pesser:dev Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants