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

Handle downstream usage issues #88

Merged
merged 14 commits into from
Nov 24, 2022
Merged

Handle downstream usage issues #88

merged 14 commits into from
Nov 24, 2022

Conversation

ChrisRackauckas
Copy link
Member

It seems this library wasn't properly tested...

@codecov
Copy link

codecov bot commented Sep 15, 2022

Codecov Report

Merging #88 (a9a82b6) into master (6347ae5) will increase coverage by 37.22%.
The diff coverage is 58.33%.

@@             Coverage Diff             @@
##           master      #88       +/-   ##
===========================================
+ Coverage    0.00%   37.22%   +37.22%     
===========================================
  Files           8        5        -3     
  Lines         459      231      -228     
===========================================
+ Hits            0       86       +86     
+ Misses        459      145      -314     
Impacted Files Coverage Δ
src/utils.jl 12.38% <12.90%> (+12.38%) ⬆️
src/raphson.jl 50.00% <60.86%> (+50.00%) ⬆️
src/NonlinearSolve.jl 100.00% <100.00%> (ø)
src/ad.jl 100.00% <100.00%> (ø)
src/jacobian.jl 43.24% <100.00%> (+43.24%) ⬆️

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

@andreasnoack
Copy link

andreasnoack commented Sep 15, 2022

The tests here are not running at all. The code coverage has been zero for more than half a year https://app.codecov.io/gh/SciML/NonlinearSolve.jl. The reason is that you only run the tests for

if GROUP == "All" || GROUP == "Interface"
but GROUP never has any of those values
- Core
- Downstream
. Might be good to add an else branch that errors.

@ChrisRackauckas
Copy link
Member Author

Finally.

@ChrisRackauckas ChrisRackauckas merged commit 5aa16a1 into master Nov 24, 2022
@ChrisRackauckas ChrisRackauckas deleted the downstream branch November 24, 2022 09:16
@andreasnoack
Copy link

Can the downstream failure safely be ignored?

@ChrisRackauckas
Copy link
Member Author

Yes, this is getting a v1.0 release, and MTK is moving to SimpleNonlinerSolve.jl. It took awhile because this library had to be split to get the dependencies correct, which is a very breaking change. It also unifies the arguments (abstol instead of tol), so it's a very breaking update. Basically, it's finally updated from being an overlooked intern project to a full compliant piece of SciML, with all of the breakage required to make it conform to the rules of the interface. The update should be relatively sane though since most things would actually just move to SimpleNonlinearSolve.jl: the "bigger" Newton here is really just for big systems which isn't used all that much downstream.

avik-pal pushed a commit that referenced this pull request Nov 1, 2024
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