-
Notifications
You must be signed in to change notification settings - Fork 519
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
docs: document more differences among different backends #4388
Conversation
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no suggestions.
📝 WalkthroughWalkthroughThe pull request introduces several enhancements to documentation files related to tensor fitting and model training across various backends, including TensorFlow, PyTorch, and JAX. Key updates include the addition of new sections that clarify backend-specific constraints and model compression options. The documents have been refined for better clarity, structure, and usability, with specific attention to descriptors such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
doc/model/train-se-e2-r.md (1)
81-81
: Consider expanding the model compression section.While the information about type embedding is useful, this section could benefit from additional details such as:
- Available compression methods
- Expected impact on model size/performance
- Any backend-specific compression considerations
- Links to relevant compression documentation
Also applies to: 82-84
doc/model/train-se-e2-a.md (1)
103-106
: LGTM! Consider adding explanations for the limitations.The backend differences are clearly documented. To make it more helpful for users, consider adding brief explanations of why these limitations exist:
- Why can't
env_protection
be non-zero in TensorFlow?- Why must
type_one_side
be true in JAX?doc/model/train-fitting-tensor.md (1)
246-249
: Enhance the backend differences sectionWhile the section correctly explains the key differences, consider these improvements for clarity:
- Explicitly list which backends (PyTorch, JAX) use
atom_exclude_types
- Add a comparative example showing both approaches:
// TensorFlow approach (include types to fit) "fitting_net": { "sel_type": [0] // Only fit type 0 (oxygen) } // Other backends approach (exclude types) "atom_exclude_types": [1] // Exclude type 1 (hydrogen)- Explain that these approaches are complementary - one specifies types to include, the other specifies types to exclude
doc/model/train-se-atten.md (2)
155-155
: Grammar: Use plural form in section headerChange "Difference among different backends" to "Differences among different backends" since multiple differences are being discussed.
160-164
: Enhance backend differences documentationWhile the constraints are clearly listed, consider enhancing this section by:
- Adding brief explanations for why these constraints exist in TensorFlow
- Clarifying what happens if users attempt to set these parameters (e.g., will it raise an error?)
- Specifying the default values that will be used in TensorFlow
For example:
In the TensorFlow backend, {ref}`scaling_factor <model[standard]/descriptor[se_atten]/scaling_factor>` cannot set to a value other than `1.0`; +Attempting to set a different value will raise a ValueError. This constraint exists due to [brief explanation].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
doc/model/train-fitting-tensor.md
(2 hunks)doc/model/train-se-atten.md
(1 hunks)doc/model/train-se-e2-a.md
(1 hunks)doc/model/train-se-e2-r.md
(1 hunks)doc/model/train-se-e3.md
(1 hunks)
🔇 Additional comments (3)
doc/model/train-se-e3.md (1)
72-73
: LGTM! Section header follows documentation style.
The new section header "Difference among different backends" is consistent with the documentation style and clearly indicates its purpose.
doc/model/train-se-e2-r.md (1)
77-80
: LGTM! Clear documentation of backend constraints.
The new section effectively documents the backend-specific limitations, using proper reference links to the documentation parameters.
doc/model/train-fitting-tensor.md (1)
33-34
: LGTM: Clear and well-referenced explanation
The text clearly explains what needs to be modified for tensor fitting, with proper references to the relevant documentation sections.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4388 +/- ##
=======================================
Coverage 84.50% 84.50%
=======================================
Files 604 604
Lines 56943 56943
Branches 3486 3488 +2
=======================================
Hits 48120 48120
Misses 7696 7696
Partials 1127 1127 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Summary by CodeRabbit
New Features
"se_atten"
and"se_atten_v2"
.Documentation
Bug Fixes