Skip to content
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

Rename samples field of WeightedIntervalTraining to points and clean tstops_loss logic #727

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

sathvikbhagavan
Copy link
Member

Renaming would be consistent with other strategies StochasticTraining, QuasiRandomTraining etc. as it contains the number of points.

Cleaning up tstops_loss to avoid redundant function evaluation.

Copy link

@ai-maintainer ai-maintainer bot left a comment

Choose a reason for hiding this comment

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

AI-Maintainer Review for PR - Rename samples field of WeightedIntervalTraining to points and clean tstops_loss logic

Title and Description 👍

The Title and Description are clear and concise
The title and description of the pull request are clear and concise. They effectively communicate the purpose of the changes, which is to rename the `samples` field of `WeightedIntervalTraining` to `points` and clean up the `tstops_loss` logic. The renaming is consistent with other strategies and the cleaning up is done to avoid redundant function evaluation.

Scope of Changes 👍

The changes are narrowly focused
The changes in the pull request are narrowly focused. The author is primarily addressing two specific issues: renaming the `samples` field of `WeightedIntervalTraining` to `points` and cleaning up the `tstops_loss` logic. These changes are related and seem to be part of a single cohesive effort to improve the codebase.

Documentation 👍

All new functions, classes, or methods have appropriate docstrings
All newly added functions, classes, or methods have docstrings describing their behavior, arguments, and return values. There are no functions, classes, or methods in the diff that need docstrings.

Testing 👎

The description does not describe how the author tested the changes
The description does not describe how the author tested the changes. It primarily focuses on explaining the purpose of the changes rather than providing details about the testing process. It would be beneficial to include information about the testing process in the description.

Suggested Changes

  • Please provide information about how you tested these changes. This could include the specific test cases you ran, any edge cases you considered, and the results of the tests.

  • Please ensure that all tests pass after your changes, including any existing tests that may be affected by your changes.

  • Please consider adding new tests to cover the changes you have made. This will help ensure that future changes do not unintentionally break this functionality.

Reviewed with AI Maintainer

@ChrisRackauckas ChrisRackauckas merged commit 5506fe4 into SciML:master Aug 28, 2023
12 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants