-
Notifications
You must be signed in to change notification settings - Fork 43
Review main-notebooks/classifier.ipynb
#122
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
base: main
Are you sure you want to change the base?
Review main-notebooks/classifier.ipynb
#122
Conversation
chienyuanchang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automated LLM code review (section-based).
LLM usage details:
- Total tokens used: 6077.
- Used deployment: cu-samples-gpt-4.1-mini
- API version: 2024-12-01-preview
| "1. Create a classifier for document categorization.\n", | ||
| "2. Create a custom analyzer to extract specific fields.\n", | ||
| "3. Combine the classifier and analyzers to classify, optionally split, and analyze documents within a flexible processing pipeline.\n", | ||
| "\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Grammar, Consistency]
- change: Added periods at the end of numbered list items.
- rationale: To conform to standard grammatical rules for complete sentences and ensure consistency across list entries.
- impact: Enhances the professionalism and readability of the documentation by presenting a polished and uniform list format.
| ">\n", | ||
| "> Fill in the constants **AZURE_AI_ENDPOINT**, **AZURE_AI_API_VERSION**, and **AZURE_AI_API_KEY** with the details from your Azure AI Service.\n", | ||
| "> Please fill in the constants **AZURE_AI_ENDPOINT**, **AZURE_AI_API_VERSION**, and **AZURE_AI_API_KEY** with the details from your Azure AI Service.\n", | ||
| "\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Grammar, Clarity]
- change: Changed the sentence from "> Fill in the constants ..." to "> Please fill in the constants ..."
- rationale: Adding "Please" makes the instruction more polite and clearer as a request rather than a command.
- impact: Improves the tone and readability of the documentation, making it more user-friendly.
| "> **💡 Note:** This step is required **only once per Azure Content Understanding resource**, unless the GPT deployment has been changed. You may skip this section if:\n", | ||
| "> - This configuration has already been completed for your resource, or\n", | ||
| "> - Your administrator has already set up the model deployments for you.\n", | ||
| "\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
categories: [Grammar, Clarity]
- change: Changed "only required once per Azure Content Understanding resource" to "required only once per Azure Content Understanding resource"
- rationale: Corrected word order for smoother, more natural English phrasing.
- impact: Improves sentence readability and comprehension.
-
categories: [Clarity, Consistency]
- change: Replaced "You can skip this section if:" with "You may skip this section if:"
- rationale: "May" implies permission more clearly and is slightly more formal and consistent for documentation tone.
- impact: Enhances the professional tone and clarity of instructions.
-
categories: [Clarity, Grammar]
- change: Changed "- This configuration has already been run once for your resource," to "- This configuration has already been completed for your resource,"
- rationale: "Completed" is a clearer and more standard term when referring to finishing a configuration step.
- impact: Reduces potential ambiguity, making instructions easier to understand.
-
categories: [Grammar, Consistency]
- change: Revised "- Your administrator has already configured the model deployments for you" to "- Your administrator has already set up the model deployments for you." (added a period)
- rationale: "Set up" is a more common and clear phrase for initial configuration; added punctuation for sentence completeness.
- impact: Improves clarity and ensures consistent formatting across list items.
| "2. Set `GPT_4_1_DEPLOYMENT`, `GPT_4_1_MINI_DEPLOYMENT`, and `TEXT_EMBEDDING_3_LARGE_DEPLOYMENT` in your `.env` file with the deployment names" | ||
| "1. Deploy **GPT-4.1**, **GPT-4.1-mini**, and **text-embedding-3-large** models in Azure AI Foundry.\n", | ||
| "2. Set `GPT_4_1_DEPLOYMENT`, `GPT_4_1_MINI_DEPLOYMENT`, and `TEXT_EMBEDDING_3_LARGE_DEPLOYMENT` in your `.env` file with the deployment names." | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
categories: [Grammar, Consistency]
- change: Added periods at the end of list items in bulleted points describing model requirements.
- rationale: To ensure proper sentence punctuation and maintain consistency across list items.
- impact: Improves readability and professionalism of the documentation by adhering to standard grammar rules.
-
categories: [Grammar, Consistency]
- change: Added periods at the end of numbered prerequisite list items.
- rationale: To maintain uniform punctuation and formal writing style within lists.
- impact: Enhances clarity and consistency, making the instructions easier to follow and visually aligned.
| " print(\" Please:\")\n", | ||
| " print(\" 1. Deploy all three models in Azure AI Foundry\")\n", | ||
| " print(\" 1. Deploy all three models in Azure AI Foundry.\")\n", | ||
| " print(\" 2. Add the following to notebooks/.env:\")\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Grammar]
- change: Added a period at the end of the printed sentence "Deploy all three models in Azure AI Foundry."
- rationale: To complete the sentence properly with correct punctuation.
- impact: Enhances the professionalism and readability of the printed message in the code.
| "source": [ | ||
| "## Clean up the created analyzer \n", | ||
| "## Clean up the created analyzer\n", | ||
| "After the demo completes, the classifier is automatically deleted to prevent resource accumulation." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Formatting]
- change: Removed the extra space before the newline character at the end of the heading line "## Clean up the created analyzer"
- rationale: Eliminating trailing whitespace ensures consistent formatting and adheres to best practices in code and documentation formatting.
- impact: Improves the cleanliness and professionalism of the documentation by avoiding unnecessary trailing spaces.
| " \"baseAnalyzerId\": \"prebuilt-document\",\n", | ||
| " \"description\": \"Loan application analyzer - extracts key information from loan applications\",\n", | ||
| " \"description\": \"Loan application analyzer - extracts key information from loan applications.\",\n", | ||
| " \"config\": {\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Grammar]
- change: Added a period at the end of the description sentence.
- rationale: The sentence lacked proper punctuation, and adding a period completes the sentence grammatically.
- impact: Improves professionalism and readability of the description.
| "\n", | ||
| "Now create a new classifier that uses the prebuilt invoice analyzer for invoices and the custom analyzer for loan application documents.\n", | ||
| "Now, create a new classifier that uses the prebuilt invoice analyzer for invoices and the custom analyzer for loan application documents.\n", | ||
| "This combines document classification with field extraction in one operation." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Grammar]
- change: Added a comma after "Now" in the sentence.
- rationale: The introductory word "Now" should be followed by a comma for correct punctuation and readability.
- impact: Improves the grammatical correctness and flow of the documentation, making it easier to read and understand.
| "else:\n", | ||
| " print(\"No enhanced analysis result available\")" | ||
| " print(\"No enhanced analysis result available.\")" | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Grammar, Consistency]
- change: Added missing periods at the end of print statement strings.
- rationale: To ensure proper sentence punctuation and maintain consistency in messaging style.
- impact: Enhances readability and professionalism of output messages, providing a consistent user experience.
notebooks/classifier.ipynb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- categories: [Formatting]
- change: Added a missing newline after the closing brace in the code.
- rationale: Ensuring proper formatting and adherence to coding style guidelines that typically require a newline at the end of files or code blocks.
- impact: Improves readability and prevents potential issues with tools or compilers that expect a final newline.
Automated review and documentation improvements for
notebooks/classifier.ipynbon branchmainLLM usage details: