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

Recompile Invalidations #2222

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Recompile Invalidations #2222

merged 2 commits into from
Sep 11, 2023

Conversation

xtalax
Copy link
Member

@xtalax xtalax commented Aug 8, 2023

Use PrecompileTools to recompile invalidations

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 - Recompile Invalidations

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 use `PrecompileTools` to recompile invalidations.

Scope of Changes 👍

The changes are narrowly focused
The changes in this pull request are narrowly focused on recompiling invalidations using the `PrecompileTools` package. There is no indication that the author is trying to resolve multiple issues simultaneously.

Documentation 👎

Missing Docstrings
The newly added functions `@recompile_invalidations` and `RuntimeGeneratedFunctions.init` do not have docstrings. It would be beneficial to add docstrings that describe their behavior, arguments, and return values.

Testing 👎

No information on testing
The description does not provide any information on how the changes were tested. It would be helpful for the author to provide information about the testing approach they used to ensure the correctness and stability of the changes.

Suggested Changes

  1. Add docstrings to the newly added functions @recompile_invalidations and RuntimeGeneratedFunctions.init to describe their behavior, arguments, and return values.
  2. Provide information on how the changes were tested. This could be in the form of unit tests, integration tests, or a description of manual tests that were performed.

Potential Issues

Without information on how the changes were tested, it's difficult to identify potential issues. However, it's always a good idea to thoroughly test new changes to ensure they don't introduce unexpected behavior or break existing functionality.

Reviewed with AI Maintainer

@ChrisRackauckas ChrisRackauckas merged commit d4095cf into SciML:master Sep 11, 2023
27 of 33 checks passed
@YingboMa
Copy link
Member

We really should not merge any PR with a format-check CI failure. It makes master not formatted properly, so all other PR's format check will fail as well.

@ChrisRackauckas
Copy link
Member

We don't really trust the formatter right now, at least until domluna/JuliaFormatter.jl#741 is solved and domluna/JuliaFormatter.jl#754 is in, but @demluna is on the case.

@YingboMa
Copy link
Member

Well, in this instance, the formatter fails because of a trailing space, and it should be fixed. https://github.com/SciML/ModelingToolkit.jl/actions/runs/6146959683/job/16677371720#step:5:22

MTK's format CI always passes before this PR, and there's not much reason to break that.

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.

3 participants