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

Given points training #722

Closed
wants to merge 8 commits into from

Conversation

sdesai1287
Copy link
Contributor

@sdesai1287 sdesai1287 commented Aug 17, 2023

#644 Initial commit for GivenPointsTraining. It works on the example I used but may be useful to test more cases. I can also remove the DAE problem stuff if necessary

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 - Given points training

Title and Description ⚠️

Title and Description could be improved
The title "Given points training" gives a hint about the changes but could be more descriptive. The description "Initial commit for GivenPointsTraining. It works on the example I used but may be useful to test more cases" is not detailed enough. It would be beneficial to provide more context about the changes, why they are necessary, and how they improve the existing codebase.

Scope 👍

The changes are narrowly focused
The changes in this PR are narrowly focused on the implementation of the GivenPointsTraining functionality. The author is not trying to resolve multiple issues simultaneously, which is a good practice as it makes the PR easier to review and understand.

Testing ⚠️

Testing details are not clear
The description does not provide enough details about how the changes were tested. It would be beneficial to include information about the specific test cases used, any edge cases considered, and the expected outcomes. This would help reviewers understand the extent of testing performed and provide confidence in the changes.

Documentation ⚠️

Some functions and methods lack docstrings
Several new functions, classes, and methods have been introduced without docstrings. These include `NNDAE`, `generate_loss_DAE`, `GivenPointsTraining`, `get_loss_function` (for `GivenPointsTraining`), `GivenPointsTraining` (in `training_strategies.jl`), and `get_loss_function` (for `WeightedIntervalTraining`). Adding docstrings that describe the behavior, arguments, and return values of these entities would improve code readability and maintainability.

Suggested Changes

  • Add more detailed information in the PR title and description about the changes and their impact.
  • Provide more details about the testing methodology and results in the PR description.
  • Add docstrings to the new functions, classes, and methods to describe their behavior, arguments, and return values.

Potential Issues

  • Without sufficient testing details, it's hard to assess the robustness of the new GivenPointsTraining functionality. More extensive testing might be needed to ensure it works correctly under different scenarios.
  • The lack of docstrings for new functions, classes, and methods could make the code harder to understand and maintain in the future.

Reviewed with AI Maintainer

@ChrisRackauckas
Copy link
Member

please do this on a fresh branch

@sdesai1287 sdesai1287 closed this Aug 17, 2023
@sdesai1287 sdesai1287 deleted the given_points_training branch August 17, 2023 17:17
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.

2 participants