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

Integrate JuliaSimCompiler #298

Merged
merged 17 commits into from
Sep 20, 2023
Merged

Integrate JuliaSimCompiler #298

merged 17 commits into from
Sep 20, 2023

Conversation

xtalax
Copy link
Member

@xtalax xtalax commented Aug 4, 2023

No description provided.

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 - Integrate JuliaSimCompiler

Title and Description ❌

Title and Description Need Improvement
The title and description of the pull request are not sufficient. They do not provide enough context or information about the purpose and motivation behind the changes. It would be beneficial for the contributor to provide a more descriptive title and a detailed description that explains the purpose and motivation behind the integration of JuliaSimCompiler.

Scope of Changes ✅

Changes are Narrowly Focused
The changes in the pull request appear to be narrowly focused on integrating JuliaSimCompiler. The modifications primarily consist of changes to the `Project.toml` file, the addition of a new module `MethodOfLinesJuliaSimCompilerExt`, and some import statements in test files. These changes are all related to incorporating JuliaSimCompiler into the existing codebase.

Documentation ❌

Missing Docstrings
There are two newly added functions that do not have docstrings:
  1. MethodOfLinesJuliaSimCompilerExt.add_metadata!
  2. MethodOfLinesJuliaSimCompilerExt.generate_system

It is recommended to add docstrings to these functions to improve code readability and maintainability. The docstrings should explain the purpose of the function, the arguments it takes, and the expected behavior.

Testing ❌

No Information on Testing
The description of the pull request does not mention anything about how the author tested the changes. It is important for the author to provide information about the testing process they followed to ensure the correctness and functionality of the integrated JuliaSimCompiler.

Suggested Changes

  1. Please provide a more descriptive title and a detailed description for the pull request.
  2. Add docstrings to the newly added functions MethodOfLinesJuliaSimCompilerExt.add_metadata! and MethodOfLinesJuliaSimCompilerExt.generate_system.
  3. Include information about how the changes were tested in the pull request description.

Potential Issues

No potential issues were identified in the code changes. However, without information on how the changes were tested, it's difficult to assess the potential impact of these changes on the existing codebase.

Reviewed with AI Maintainer

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #298 (d88bf45) into master (6eb2984) will decrease coverage by 2.06%.
Report is 3 commits behind head on master.
The diff coverage is 8.69%.

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
- Coverage   86.17%   84.11%   -2.06%     
==========================================
  Files          38       39       +1     
  Lines        1721     1763      +42     
==========================================
  Hits         1483     1483              
- Misses        238      280      +42     
Files Changed Coverage Δ
...SimCompilerExt/MethodOfLinesJuliaSimCompilerExt.jl 0.00% <0.00%> (ø)
src/MethodOfLines.jl 100.00% <ø> (ø)
src/MOL_discretization.jl 60.56% <100.00%> (ø)
...ization/schemes/function_scheme/function_scheme.jl 73.68% <100.00%> (ø)
src/interface/MOLFiniteDifference.jl 90.90% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

xtalax and others added 8 commits August 4, 2023 18:17
* add precompilation stage

* merge precompiliation discretizations

* fix

* fix #293

* Update Project.toml

* CompatHelper: add new compat entry for PrecompileTools at version 1, (keep existing compat)

* Try new scheme for mixed derivs

* remove show

* mark broken

* init mixed deriv rules

* Update Project.toml

---------

Co-authored-by: CompatHelper Julia <[email protected]>
ChrisRackauckas and others added 4 commits September 20, 2023 08:19
Add a new workflow, `JuliaSimCompiler.yml`, which tests for integration
with JuliaSimCompiler. Since PRs opened from forks don't have access to
secrets, turn on `fail-fast` for these tests as well, so that it doesn't
mess with the CI statuses for such PRs.
Remove `JuliaSimCompiler` as a test dependency as that would require
having access to the package at all times to run the tests, making it
synonymous with a direct dependency (and less of an extension) in the
context of running tests.
Instead, explicitly `Pkg.add` `JuliaSimCompiler` instead in the
relevant test matrices.
@xtalax
Copy link
Member Author

xtalax commented Sep 20, 2023

@thazhemadam the integration test is just exiting by the looks of things, bad config?

@thazhemadam
Copy link
Member

@xtalax That's the fail-fast configuration specified for the workflow kicking in.
The job itself is failing because the PR was opened from a fork and GitHub doesn't provide access to GitHub secrets for CI jobs running on PRs opened from forks (for security reasons), as a result of which the job can't pick up the required secrets.

@xtalax
Copy link
Member Author

xtalax commented Sep 20, 2023

@thazhemadam good to know, does that mean we're good to merge? Other fails are timeouts.

@thazhemadam
Copy link
Member

@xtalax yes :shipit:

@xtalax xtalax merged commit 5f33fe0 into SciML:master Sep 20, 2023
36 of 46 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.

3 participants