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

backward integration bug #58

Merged
merged 3 commits into from
Dec 26, 2023
Merged

backward integration bug #58

merged 3 commits into from
Dec 26, 2023

Conversation

mikelehu
Copy link
Contributor

  • Now, RKGaussLegendre.jl works well for backward integrations.
  • I have added a new notebook called "/Examples/Simple Pendulum- Forward and Back integration.ipynb where I have tested the implementation

Copy link

codecov bot commented Dec 23, 2023

Codecov Report

Attention: 76 lines in your changes are missing coverage. Please review.

Comparison is base (38f1ca4) 25.44% compared to head (a84c095) 26.36%.

Files Patch % Lines
src/IRKGL16step_adaptive_par.jl 0.00% 24 Missing ⚠️
src/IRKGL16step_fixed_par.jl 0.00% 18 Missing ⚠️
src/IRKGL16step_adaptive_seq.jl 29.16% 17 Missing ⚠️
src/IRKGL16step_fixed_seq.jl 33.33% 12 Missing ⚠️
src/IRKGL16Solver.jl 76.19% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   25.44%   26.36%   +0.92%     
==========================================
  Files           8        8              
  Lines        1973     1995      +22     
==========================================
+ Hits          502      526      +24     
+ Misses       1471     1469       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -0,0 +1,334 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

this file isn't needed

@ChrisRackauckas
Copy link
Member

No test?

@mikelehu
Copy link
Contributor Author

Hi,

Should I be worried about failed checks? Or will the pull request be merged into the base branch and made public in the next few days?

Thanks.

@ChrisRackauckas
Copy link
Member

Those code coverage decreases are just saying that the new code isn't being tested.

@mikelehu
Copy link
Contributor Author

I don't understand what you want to tell me. I have modified the implementation to resolve the issue raised by Andreas Varga.
issue 42

After confirming locally that the new deployment is correct, I made the pull request.
Do I delete the current pull request and create a new one?

@ChrisRackauckas
Copy link
Member

You can add more commits to the same branch and it will register in this pull request. My point though is that the Jupyter notebook is not part of the test suite and it would be good to ensure that this feature gets a proper test. The only things that are tested are the files that are in the test/runtests.jl file. https://github.com/SciML/IRKGaussLegendre.jl/blob/master/test/runtests.jl I think if you paste the reverse time solve example into there with a quick test on it then this is good to go.

@mikelehu
Copy link
Contributor Author

You are right, I agree that it is important to add a proper test for reverse time solve.
I have pasted an the example of the Jupyter Notebook in the runtests.jl file.
I hope it's enough, thanks for your help.

test/runtests.jl Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit 735b5a9 into SciML:master Dec 26, 2023
5 of 6 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.

2 participants