-
Notifications
You must be signed in to change notification settings - Fork 0
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
update docs #18
update docs #18
Conversation
Reviewer's Guide by SourceryThis pull request enhances the Sequence diagram for training step timing measurementsequenceDiagram
participant Client
participant Trainer
participant TrainState
Client->>Trainer: compile(measture_train_step_compile_time=true)
activate Trainer
Note right of Trainer: Start compile time measurement
Trainer->>Trainer: _compile_training_step()
Trainer->>TrainState: set_data_train()
Trainer->>Trainer: fn_train_step.compile()
Note right of Trainer: End compile time measurement
Trainer-->>Client: return compile_time
deactivate Trainer
Client->>Trainer: train(measture_train_step_time=true)
activate Trainer
Note right of Trainer: Start execution time measurement
Trainer->>Trainer: _train()
Note right of Trainer: Training iterations
Note right of Trainer: End execution time measurement
Trainer-->>Client: return execution_time
deactivate Trainer
Class diagram for Trainer modificationsclassDiagram
class Trainer {
+compile(optimizer, metrics, measture_train_step_compile_time)
+train(iterations, display_every, batch_size, callbacks, model_restore_path, model_save_path, measture_train_step_time)
-_compile_training_step(batch_size)
-_train(iterations, display_every, batch_size, callbacks)
}
note for Trainer "Added timing measurement capabilities"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @chaoming0625 - I've reviewed your changes - here's some feedback:
Overall Comments:
- There's a typo in the parameter names: 'measture' should be 'measure' in both the compile() and train() methods
- The commit message 'update docs' doesn't accurately reflect the changes being made - this PR includes functional changes to the trainer class and code reorganization beyond just documentation updates
- Consider implementing the timing functionality using a profiling decorator or callback system instead of adding it directly to the training loop, which would keep the core code cleaner
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -74,6 +75,7 @@ def compile( | |||
self, | |||
optimizer: bst.optim.Optimizer, | |||
metrics: Union[str, Sequence[str]] = None, | |||
measture_train_step_compile_time: bool = False, |
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.
issue (typo): Parameter name contains a typo: 'measture' should be 'measure'
Suggested implementation:
measure_train_step_compile_time: bool = False,
if measure_train_step_compile_time:
You may need to:
- Update any documentation strings that reference this parameter
- Update any calls to this method that explicitly name this parameter
t0 = time.time() | ||
self._compile_training_step(self.batch_size) | ||
t1 = time.time() | ||
return self, t1 - t0 |
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.
issue (bug_risk): Inconsistent return types could cause bugs - method returns tuple when measuring time but self otherwise
Consider using a consistent return type and providing the timing information through a different mechanism, such as a class attribute or logging.
This pull request includes several changes to improve code readability, functionality, and performance. The most important changes include modifications to the
pinnx/_trainer.py
file to add new functionality for measuring training step compile and execution times, and improvements to thedocs/unit-examples-forward/Beltrami_flow.py
anddocs/unit-examples-forward/heat.ipynb
files for better code structure and clarity.Functionality Enhancements:
pinnx/_trainer.py
: Added functionality to measure training step compile time and execution time by introducing new parametersmeasture_train_step_compile_time
andmeasture_train_step_time
in thecompile
andtrain
methods respectively. [1] [2]Code Structure Improvements:
docs/unit-examples-forward/Beltrami_flow.py
: Improved readability by reformatting the dictionaryr
in theicbc_cond_func
function.docs/unit-examples-forward/heat.ipynb
: Removed an unnecessary import statement and fixed a lambda function to improve code clarity. [1] [2]Import Adjustments:
pinnx/_trainer.py
: Addedimport time
to support the new timing functionality.pinnx/geometry/geometry_2d.py
: Removed an unused importjax.numpy
and addedfrom pinnx import utils
for better code organization.Summary by Sourcery
Add functionality to measure training step compile and execution times, and improve code readability in documentation examples.
New Features:
Tests: