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

for repo review #31

Merged
merged 116 commits into from
Aug 11, 2021
Merged

for repo review #31

merged 116 commits into from
Aug 11, 2021

Conversation

HLasse
Copy link
Collaborator

@HLasse HLasse commented Aug 11, 2021

No description provided.

augmenty/augment_utilities.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@HLasse HLasse left a comment

Choose a reason for hiding this comment

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

great work my man! very clear and concise code, well documented and easy to follow along.

a couple of points:

  • It would be optimal if yield_original when used in combination with set_doc_level did not yield the same Example twice if no augmentation took place.
  • An extra getting started/appetizer section would help new users adopt easier. The notebook is great, but it takes some effort to get there, and some users might get lost/impatient before working through the example. Perhaps a very short example directly in the README so users do not have to refer to the documentation for an API example
  • we should consider adding a tutorial showing how to train a new spaCy model using the augmenters (also for own reference)
  • relatedly, we should consider making a couple of sample combinations of augmenters with sensible defaults for model training/model evaluation (could be part of the tutorial)
  • check the comments for a couple of other small notes

I will give the readme/docs/notebook a read through for grammar tomorrow

augmenty/character/casing.py Outdated Show resolved Hide resolved
augmenty/character/replace.py Outdated Show resolved Hide resolved
augmenty/character/replace.py Show resolved Hide resolved
augmenty/character/swap.py Outdated Show resolved Hide resolved
augmenty/meta.json Show resolved Hide resolved
augmenty/token/swap.py Outdated Show resolved Hide resolved
dev/all_augmenters.py Outdated Show resolved Hide resolved
dev/todolist.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
augmenty/augment_utilities.py Show resolved Hide resolved
KennethEnevoldsen and others added 8 commits August 11, 2021 15:21
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
Co-authored-by: HLasse <lasseh0310@gmail.com>
@KennethEnevoldsen
Copy link
Owner

KennethEnevoldsen commented Aug 11, 2021

It would be optimal if yield_original when used in combination with set_doc_level did not yield the same Example twice if no augmentation took place.

Added in the newest commit as we discussed.

An extra getting started/appetizer section would help new users adopt easier. The notebook is great, but it takes some effort to get there, and some users might get lost/impatient before working through the example. Perhaps a very short example directly in the README so users do not have to refer to the documentation for an API example

Added in the newest commit.

we should consider adding a tutorial showing how to train a new spaCy model using the augmenters (also for own reference)

Added an issue #33

relatedly, we should consider making a couple of sample combinations of augmenters with sensible defaults for model training/model evaluation (could be part of the tutorial)

Agree. Let us do it when we have a bit more augmenters.

@KennethEnevoldsen KennethEnevoldsen merged commit c5e434a into review/augmenty_review Aug 11, 2021
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