Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Adding the notebook for quick_start/train.py #157

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ekdnam
Copy link

@ekdnam ekdnam commented Feb 27, 2021

currently only the code has been added.

the information and content is yet to be added.

currently only the code has been added. TODO: adding the info
@ekdnam
Copy link
Author

ekdnam commented Feb 27, 2021

The notebook can be viewed here (incase there are problems on github): nbviewer.jupyter.org

@ekdnam
Copy link
Author

ekdnam commented Feb 27, 2021

(For allenai/allennlp#5024)

@epwalsh epwalsh self-assigned this Feb 27, 2021
@epwalsh
Copy link
Member

epwalsh commented Mar 1, 2021

This looks really good in notebook format!

My one concern is that we'd have two copies of the same information (in the notebook vs in the original guide chapter), which means more difficulty in maintaining.

That said, we are striving hard to maintain backwards compatibility moving forward, so it's unlikely we'll have to update any of this anytime soon.

@matt-gardner any concerns?

@matt-gardner
Copy link
Contributor

I don't have any particular objections, because I'm not the one maintaining it =). Seems pretty awkward to be making identical changes in two places if you want to change any content in the guide (assuming you do this for all of the chapters). It doesn't bother me one way or the other if you want to go this route, but finding a way to make one of these auto-generated from the other seems like a much more scalable solution. On the other hand, there are print statements and other binder-specific considerations that you might want to remove when the code goes into a notebook that you run on your own machine, or on colab or something.

@ekdnam
Copy link
Author

ekdnam commented Mar 1, 2021

This looks really good in notebook format!

Thanks @epwalsh!

My one concern is that we'd have two copies of the same information (in the notebook vs in the original guide chapter), which means more difficulty in maintaining.

Once I began the work, I felt the same

@ekdnam
Copy link
Author

ekdnam commented Mar 1, 2021

... but finding a way to make one of these auto-generated from the other seems like a much more scalable solution.

I agree @matt-gardner.

On the other hand, there are print statements and other binder-specific considerations that you might want to remove when the code goes into a notebook that you run on your own machine, or on colab or something.

Noted

@ekdnam
Copy link
Author

ekdnam commented Mar 1, 2021

I will look into how the notebooks can be auto-generated. Thanks a lot for the suggestion @matt-gardner!

While writing the content for the colab, I noticed some discrepancies in the code, its explanation on the website, and the current code in allenai\allennlp-guide\quick_start.

For example, the tokenizer in ClassificationTsvReader is tokenizer or WhitespaceTokenizer(), but in the guide it is SpacyTokenizer(). The class in the guide has @DatasetReader.register('classification-tsv') , but not so in the actual code.

I am going to make a list of the discrepancies observed once the train.py notebook would be ready, and get your insights on it once it is done!

@ekdnam
Copy link
Author

ekdnam commented Mar 6, 2021

@epwalsh can you please once go through your_first_model.ipynb?

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

So far I just looked over the your_first_model.ipynb, at it looks really good. I just left a couple minor comments.

"\r\n",
"![dataset-reader.png](https://raw.githubusercontent.com/allenai/allennlp-guide/master/static/part1/your-first-model/dataset-reader.svg)\r\n",
"\r\n",
"The first step for building an NLP application is to read the dataset and represent it with some internal data structure.\r\n",
Copy link
Member

Choose a reason for hiding this comment

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

The phrase "The first step for building" is also used in the previous section, so it sounds a little repetitive here.

"id": "DQVGPAu0A_iA"
},
"source": [
"# class SimpleClassifier(Model)"
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit weird as the header.. maybe could just call this section "Putting it all together" or something like that.

@ekdnam
Copy link
Author

ekdnam commented Mar 19, 2021

@epwalsh thanks a lot for your review! I am very sorry for the late reply. Our study workload at the college has increased, and I have not been able to give much attention to any work outside of college. Once it gets completed, I will begin working on your suggestions immediately.

@epwalsh
Copy link
Member

epwalsh commented Mar 19, 2021

No rush @ekdnam!

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

Successfully merging this pull request may close these issues.

3 participants