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

New package: NonlinearSolvers v0.1.0 #26026

Merged

Conversation

JuliaRegistrator
Copy link
Contributor

@JuliaRegistrator JuliaRegistrator commented Dec 8, 2020

JuliaRegistrator referenced this pull request in CliMA/NonlinearSolvers.jl Dec 8, 2020
10: Change name to NonlinearSolvers r=charleskawczynski a=charleskawczynski



Co-authored-by: Charles Kawczynski <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2020

Your new package pull request does not meet the guidelines for auto-merging. Please make sure that you have read the General registry README. The following guidelines were not met:

  • Package name similar to 1 existing package.
  1. Similar to NonlinearSolve. Damerau-Levenshtein distance 2 is at or below cutoff of 2.

Note that the guidelines are only required for the pull request to be merged automatically. However, it is strongly recommended to follow them, since otherwise the pull request needs to be manually reviewed and merged by a human.

Since you are registering a new package, please make sure that you have also read the package naming guidelines: https://julialang.github.io/Pkg.jl/dev/creating-packages/#Package-naming-guidelines-1


If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment.

UUID: f4b8ab15-8e73-4e04-9661-b5912071d22b
Repo: https://github.com/CliMA/NonlinearSolvers.jl.git
Tree: 13de2bfa3716485129b59d2fdc1c48904c2cfa15

Registrator tree SHA: e934b8c55381f28735124f23e8f7e96d09b20416
@JuliaRegistrator JuliaRegistrator force-pushed the registrator/nonlinearsolvers/f4b8ab15/v0.1.0 branch from 2982fef to f3c2e53 Compare December 8, 2020 03:53
JuliaRegistrator referenced this pull request in CliMA/NonlinearSolvers.jl Dec 8, 2020
11: Add julia compat r=charleskawczynski a=charleskawczynski



Co-authored-by: Charles Kawczynski <[email protected]>
@charleskawczynski
Copy link

Unfortunately, NonlinearSolve.jl is not GPU friendly with ForwardDiff. So, I would like to continue with registering NonlinearSolvers.jl

@christopher-dG
Copy link
Member

christopher-dG commented Dec 14, 2020

Is it not possible to add the desired GPU support to the existing package?

@charleskawczynski
Copy link

Admittedly, I didn't spend too much time trying. I'm sure it's possible, but I've not allocated much time for the task that requires this functionality. Also, NonlinearSolve.jl does not appear to tested on GPUs, and may easily lose GPU support without having GPU tests.

@charleskawczynski
Copy link

charleskawczynski commented Dec 14, 2020

I could allocate some time to try to fix the support but, unless someone can help configure GPU CI, I'd rather not get stuck in a version deadlock vs GPU support.

@christopher-dG
Copy link
Member

@maleadt can help with CI, or me if he's busy.

@maleadt
Copy link
Contributor

maleadt commented Dec 14, 2020

Sure, I can have a look. What exactly is the problem wrt. CI?

@charleskawczynski
Copy link

GPU CI is non-existent, AFAICT

@StefanKarpinski
Copy link
Contributor

StefanKarpinski commented Dec 14, 2020

It's a bit unfortunate to have two packages with such similar goals, but sometimes it's unavoidable. It would be great if you can try, in the future, to converge these somehow, but in the meantime, I think the names NonlinearSolve and NonlinearSolvers are sufficiently different to both be registered.

@charleskawczynski
Copy link

I completely agree.

I can try and help to fix GPU support for NonlinearSolve.jl in the long run. I'm definitely keen to help out more in the ecosystem.

@christopher-dG
Copy link
Member

Just seems unfortunate that if/when that happens, this package will probably be deprecated/abandoned and still super easy to accidentally install.

@StefanKarpinski
Copy link
Contributor

We can continue work with whichever of the two packages we like the name of better (personally I like this one).

@charleskawczynski
Copy link

I went to dig into NonlinearSolve.jl's issue but, I realized that there's also a separate issue related to the interface. NonlinearSolve.jl uses keyword arguments for some important parameters we'll need access to. Unfortunately, we've observed elusive performance issues with kwargs inside GPU kernels. Personally, I'd really like to fix this issue because it may really help clean up some of our interfaces, but I've been unsuccessful in writing an MWE that captures the problem.

@ChrisRackauckas
Copy link
Member

but in the meantime, I think the names NonlinearSolve and NonlinearSolvers are sufficiently different to both be registered.

I think it would be good for the NLSolvers org to have domain over the NonlinearSolvers name. We made sure to make it a two variation difference. NonlinearSolvers is a one variation difference from two entities, one of which is hosts some of the most used Julia packages (NLsolve.jl, Optim.jl), and the other of which is already a dependency of some of the most used Julia packages (through DiffEqBase.jl). It is essentially the one variation of the naming that wasn't hit, but that makes me a bit weary about it...

Also, NonlinearSolve.jl does not appear to tested on GPUs, and may easily lose GPU support without having GPU tests.

I think it's clear from the history of SciML that there is not only an intention to add GPU CI and GPU support here, but to heavily support the GPU CI for this project as a good chunk of the CI bots are hosted by us and we plan to add even more.

The first goal here was to essentially deprecate Roots.jl because it's hilariously bad: it's made for scalar rootfinding but it allocates on every function call because it's type-unstable and uses dictionaries everywhere. It's more than 6x faster and non-allocating there. Next, for the NeuralSim project, was to make its static array Newton as non-allocating as fast as possible. Now we're building up to the next phase the moment we hit a problem where nonlinear tearing isn't able to reduce the size of the system enough. The issue is that we're using methods that are pretty optimized, so we transform 8,000 equation systems into scalar or 2-dimensional rootfinding problems to solve large stiff ODE systems fast, so with these methods it's going to take a bit before we get to a real system that generates even a 20-term nonlinearity. But we're building up, and we'd like to work with anyone who wants to build a full alternative to NLsolve that is fully pre-cached and GPU compatible. (Of course, both NonlinearSolve.jl and NonlinearSolvers.jl would be inefficient to use within an ODE solver so that's a whole different ballgame, so this is about reduced forms).

NonlinearSolve.jl uses keyword arguments for some important parameters we'll need access to. Unfortunately, we've observed elusive performance issues with kwargs inside GPU kernels.

Closures and dropping down one step won't fix the problem? There's other ways to do this that we can discuss.

Anyways, I'm happy to work with anyone here. Getting this to be GPU compatible is probably a few steps away.

Also BTW, this isn't going to be your main issue getting these tools into kernels. The main GPU issue you'll have is https://github.com/CliMA/NonlinearSolvers.jl/blob/master/src/newton_method_ad.jl#L56-L57 trying to have a dynamically-sized history vector that you push into won't work on a kernel, so you'd want to change how the history is handled by making it on the caller side. You can do that by using an iterator interface instead, so that the user can inject code between iterations. NonlinearSolve.jl happens to have an iterator interface, and that's indeed otherwise it wouldn't compose well with https://github.com/SciML/DiffEqGPU.jl where it's used in the event handling interface.

@StefanKarpinski
Copy link
Contributor

👍🏻. In any case, it seems like we should go ahead with this as a separate package for now? You still want this merged, @charleskawczynski?

@charleskawczynski
Copy link

I think it would be good for the NLSolvers org to have domain over the NonlinearSolvers name

I think this makes sense in the long run.

The first goal here was to essentially deprecate Roots.jl...

Makes sense. We originally tried using Roots.jl but, IIRC, we ran into issues and ended up writing RootSolvers.jl, which is just the scalar-counterpart to NonlinearSolvers.jl.

Closures and dropping down one step won't fix the problem? There's other ways to do this that we can discuss. Anyways, I'm happy to work with anyone here. Getting this to be GPU compatible is probably a few steps away.

I'm not sure if it will fix the problem. I agree that there may be other ways, but it might be awkward / result in duplicate code. I think the right/clean thing to do is make a reproducer of the observed problem, fix the issue, and then use a kwarg interface but I'd need help with this. For the short term, I would prefer to avoid the uncertainty of this timeline.

The main GPU issue you'll have is ...

That function is only for CPU debugging and has do nothing behavior for CompactSolution, which is what we'll use for GPU runs.

You can do that by using an iterator interface instead, so that the user can inject code between iterations.

We thought about this and didn't add it for no particular reason-- I think this would be a nice improvement over what we currently have.

You still want this merged, @charleskawczynski?

Ultimately, yes. Would it make sense to register under the NLSolvers org to start with? or transfer later? I think I'd need admin privileges to add/register under the NLSolvers org.

@charleskawczynski
Copy link

Actually, that would require reconfiguring the CI infrastructure. I prefer to just merge this.

@StefanKarpinski
Copy link
Contributor

Do you want to tag a newer commit as 0.1.0 or merge this as is?

@charleskawczynski
Copy link

I'm fine with merging this as is--we haven't added any newer commits.

@StefanKarpinski StefanKarpinski merged commit 238e9fd into master Jan 4, 2021
@StefanKarpinski StefanKarpinski deleted the registrator/nonlinearsolvers/f4b8ab15/v0.1.0 branch January 4, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants