-
Notifications
You must be signed in to change notification settings - Fork 9
Comprehensive editorial review of book manuscript #41
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: book
Are you sure you want to change the base?
Comprehensive editorial review of book manuscript #41
Conversation
|
@copilot This is great. Good job @copilot ! Now make one more pass (in this PR) over the text to review the language, flow, clarity and any other aspects of the text you deem important for the academic textbook. Mark your comments/suggestions/questions in the text with proper comments for the human autors and clear, easy to find markers. |
… and Chapter 1 Co-authored-by: jochym <5993422+jochym@users.noreply.github.com>
Co-authored-by: jochym <5993422+jochym@users.noreply.github.com>
…comments Co-authored-by: jochym <5993422+jochym@users.noreply.github.com>
I've completed a comprehensive editorial review of the book manuscript (main.tex). The review included: Changes Made:
Editorial Comments Added (28 total):
Key Findings: |
|
@copilot how to add a new person to the project? |
To add a new person to the GitHub project, you'll need to:
Collaborators can be given different permission levels:
Note: Only repository owners and admins can add new collaborators. For this specific repository (jochym/abinitio-methods), you would need appropriate permissions to access these settings. |
book/main.tex
Outdated
| \usepackage[left=2cm,right=2cm,top=3cm,bottom=3cm]{geometry} | ||
| \DeclareMathOperator{\Tr}{Tr} | ||
|
|
||
| % **[EDITORIAL]**: Mathematical notation - The manuscript uses both \mb (defined as \bm) and direct \bm commands for bold math. For consistency, consider using only one convention throughout. The \mb macro is convenient but \bm is more standard and immediately recognizable. |
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.
Replace \mb with \bm .
| \chapter*{Introduction} | ||
| \addcontentsline{toc}{chapter}{Introduction} | ||
|
|
||
| % **[EDITORIAL]**: Introduction - Overall structure: Excellent historical overview. Consider adding a roadmap paragraph at the end to outline the book's chapter structure and guide readers through the content progression. |
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.
Create the proposal for such roadmap paragraph.
| % **[EDITORIAL]**: Line 45 - "enabled to explain the basis properties" - should be "enabled explanation of the basic properties" (grammar: "enabled to" is incorrect; "basis" should be "basic") | ||
| First principles calculation methods, also called {\it ab initio} methods, are based on the laws of quantum mechanics. | ||
| The application of the Schr\"{o}dinger equation~\cite{Schrodinger} to atoms and molecules enabled to explain the basis properties of orbitals and chemical bonds~\cite{HL,hartree28,mulliken,JC}. | ||
| The application of the Schr\"{o}dinger equation~\cite{Schrodinger} to atoms and molecules enabled explanation of the basic properties of orbitals and chemical bonds~\cite{HL,hartree28,mulliken,JC}. |
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.
OK
book/main.tex
Outdated
| and from the repulsive part coming from the remaining electrons. | ||
| Since the electronic part of the potential depends on the wave functions we want to obtain, | ||
| the Schr\"{o}dinger equation must be solved in a self-consistent manner. | ||
| % **[EDITORIAL]**: Lines 61-62 - Awkward sentence structure with "is written is in the form" - rewrite for clarity |
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.
Much better.
book/main.tex
Outdated
| which leads to significantly overestimated values of the energy gap in solids. | ||
| Methods based on the Hartree-Fock approach, the so-called multi-determinant methods, correctly take into account electronic correlations, but require time-consuming computer calculations. They are mainly used in quantum chemistry to study molecular systems. | ||
| % **[EDITORIAL]**: Line 70 - Run-on sentence needs better structure with semicolon or period | ||
| Methods based on the Hartree-Fock approach, the so-called multi-determinant methods, correctly take into account electronic correlations; however, they require time-consuming computer calculations. They are mainly used in quantum chemistry to study molecular systems. |
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.
I prefer shorter sentences reformulate with long sentence split.
| % **[EDITORIAL]**: Line 72 - "periodic atomic potential" should be "a periodic atomic potential" (article needed) | ||
| The first calculations of the electronic structure of metals used methods in which independent electrons interact | ||
| with periodic atomic potential. | ||
| with a periodic atomic potential. |
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.
OK
book/main.tex
Outdated
| spherically symmetric potentials generated by each atom. | ||
| Using Bloch's theorem, all we need to do is find solutions to the Schr\"{o}dinger equation | ||
| in a single primitive cell, with appropriate boundary conditions at the cell boundary. | ||
| % **[EDITORIAL]**: Lines 78-79 - "the entire crystal is divided to the areas" should be "divided into areas" (preposition error) |
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.
OK
| However, in the interstitial area, both the potential and wave functions are slowly changing, and the natural basis are plane waves. | ||
| Additionally, the condition of continuity at the border of areas must be met. | ||
| % **[EDITORIAL]**: Line 82 - "the natural basis are plane waves" should be "the natural basis is plane waves" or "plane waves are the natural basis" (subject-verb agreement) | ||
| However, in the interstitial area, both the potential and wave functions are slowly changing, and plane waves are the natural basis. |
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.
OK
| % **[EDITORIAL]**: Line 82 - "the natural basis are plane waves" should be "the natural basis is plane waves" or "plane waves are the natural basis" (subject-verb agreement) | ||
| However, in the interstitial area, both the potential and wave functions are slowly changing, and plane waves are the natural basis. | ||
| % **[EDITORIAL]**: Line 83 - "at the border of areas" should be "at the boundaries between areas" (more precise terminology) | ||
| Additionally, the condition of continuity at the boundaries between areas must be met. |
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.
OK
book/main.tex
Outdated
| of electron density, which can be determined by minimizing the ground state energy. | ||
| In practical applications, the electron density is determined from single-particle wave functions, calculated as | ||
| self-consistent by solving the Kohn-Sham equation. | ||
| % **[EDITORIAL]**: Line 99 - "calculated as self-consistent" is awkward; should be "calculated self-consistently" |
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.
OK
|
@copilot I have done the cherry-picking and push from cli. Can you verify that the accepted commits are indeed in book branch and are not in this PR any more? |
I can verify that all 6 accepted commits (c95da1e through 45469d3) are still present in the current PR branch: However, I cannot directly access the To verify the cherry-pick worked correctly, you can:
If the cherry-pick was successful, you should see the same commit messages in the book branch, though they may have different SHAs if git created new commits during the cherry-pick process. The commits remain in this PR branch because git cherry-pick copies commits rather than moving them. |
|
@copilot Can you verify on which commit to book branch this PR is based at the moment? |
This PR branch is based on commit 585c817 (grafted), which is from PR #39 that translated Chapter 9 to English. The "(grafted)" notation indicates this is a shallow clone where the full git history beyond this point is not available. This commit represents the state of the book branch at the time this PR was created. Current PR structure: The PR contains 8 commits (88139bc through 45469d3) built on top of commit 585c817 from the book branch. Since the repository is grafted, I cannot see the full history of the book branch beyond this point. |
|
@copilot Can you rebase the PR to the current state of the book branch? |
I cannot perform a git rebase operation. My capabilities are limited to:
I do not have access to:
To rebase this PR to the current book branch, you'll need to: # Fetch latest changes
git fetch origin book
# Checkout your PR branch
git checkout copilot/sub-pr-25-db015754-b179-439a-9233-7a3321265e61
# Rebase onto book
git rebase origin/book
# Force push (if rebase successful)
git push --force-with-lease origin copilot/sub-pr-25-db015754-b179-439a-9233-7a3321265e61Note: Since you've already cherry-picked commits c95da1e through 45469d3 into the book branch, the rebase may create conflicts or duplicate commits. You might want to either:
|
|
@copilot I like the third option. |
I cannot perform git reset or rebase operations directly. To reset this PR to only contain unreviewed editorial markers, you'll need to: Manual approach: # Fetch the latest book branch
git fetch origin book
# Reset your PR branch to match book
git checkout copilot/sub-pr-25-db015754-b179-439a-9233-7a3321265e61
git reset --hard origin/book
# Force push to update the PR
git push --force-with-lease origin copilot/sub-pr-25-db015754-b179-439a-9233-7a3321265e61Then I can help add back only the unreviewed editorial markers/suggestions. Alternative approach I CAN do:
This would give you a PR with only the remaining editorial suggestions. Would you like me to proceed with this alternative approach? |
|
@copilot Yes. Do the alternative approach but use a separate PR based on book branch. We are not sure if this will not mess things up. Please try this. |
I cannot create a separate PR or access the book branch. My capabilities are limited to:
I do not have access to:
What I CAN do in THIS PR: What YOU need to do for a separate PR: # Create new branch from book
git checkout book
git pull origin book
git checkout -b editorial-markers-only
# Then I can help you add the editorial markers in that new PRWould you like me to transform this current PR to only contain unreviewed markers, or would you prefer to create a new branch manually and then I can help add the markers there? |
|
@copilot This whole cherry-picking was ill-advised. Can you instruct me how to reset book branch to the state this PR was based on? |
To reset the book branch to the state before the cherry-picked commits (commit 585c817), you'll need to: # First, find the commit SHA before the cherry-picks
# (the last commit before your cherry-pick operation)
git checkout book
git log --oneline -10 # Find the commit before the cherry-picks
# Reset book branch to 585c817 (or the commit just before your cherry-picks)
git reset --hard 585c817f12ba72da61dff7a8b8addc612b2e45d9
# Force push to update the remote book branch
git push --force-with-lease origin bookImportant: If there were other commits on book after 585c817 but before your cherry-picks, you'll need to identify the correct commit. Use Alternative (safer if unsure): # Create a backup first
git checkout book
git branch book-backup
# Then use git reflog to find the state before cherry-pick
git reflog show book
# Look for the entry before your cherry-pick operation
# Then reset to that commit SHA
git reset --hard <commit-sha-before-cherry-pick>
git push --force-with-lease origin bookAfter resetting book, this PR will still contain all the work, and you can merge it normally to apply both the accepted changes and the editorial markers together. |
|
Ok @copilot now book branch is at the same commit as the base of this PR. Please summarize (after all this mess above) what is done and what ramains to be done in this PR. Update the message (plan) on top of the PR with this information. I have a major co-author who is not experienced with github and code review in PR. Plese include concise introduction for him - I just requested review of this PR from him. Add information what he should do in the web interface to execute his review. |
Updated PR description with comprehensive summary and co-author review instructions. The PR now clearly shows:
Included step-by-step GitHub review instructions for your co-author who is less familiar with the interface. The book branch is now at the same base commit (585c817) as this PR, so merging will apply all changes cleanly. |
Comprehensive Editorial Review of Book Manuscript
For Co-Author Reviewers: How to Review This PR
Welcome! This pull request contains editorial improvements to the book manuscript (
book/main.tex). Here's how to review it on GitHub:% **[EDITORIAL]**:- these need your verificationSummary of Work Completed
This PR performs a comprehensive editorial review of the entire book manuscript (4,234 lines) focusing on:
✅ Changes Applied and Accepted (commits c95da1e → 45469d3)
Grammar & Terminology Fixes (20+ corrections):
Mathematical Notation Standardization:
\mbwith\bmthroughout document\mbmacro definition\ne→\neqin crystal systems tableCritical Corrections:
Structural Additions:
🔍 Items Requiring Co-Author Verification
The following editorial markers remain in the text for your review:
Chapter 1 - Electron Interactions:
Chapter 2 - Electronic States:
Chapter 3 - Density Functional Theory:
Chapter 9 - Many-Body Effects:
General:
Plus 21 review markers throughout chapters for technical terminology verification
📊 Statistics
✨ Overall Assessment
The manuscript is well-structured and suitable for PhD students in computational solid state physics. It successfully balances theoretical foundations with practical computational methods. The editorial review improved clarity while maintaining technical accuracy. All substantive changes have been made; remaining markers are suggestions for consideration by the authors.
Next Steps
% **[EDITORIAL]**:)💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.