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

Separate Poisson solvers from main code #1553

Closed
tomchor opened this issue Apr 7, 2021 · 9 comments
Closed

Separate Poisson solvers from main code #1553

tomchor opened this issue Apr 7, 2021 · 9 comments

Comments

@tomchor
Copy link
Collaborator

tomchor commented Apr 7, 2021

Okay, so I'm just gonna throw this out there: is it advantageous to migrate the Poisson solvers to a separate package (PoissonSolvers.jl)? It would be akin to the relationship between PencilFFTs.jl and PencilArrays.jl. I've been thinking about it for the past few days and I can see some pros:

  • Separating the code can make Oceananigans easier to maintain:
    • fewer things to test in every PR (the tests are becoming larger and larger and apparently we're starting to have backlogs on buildkite)
    • fewer lines/modules in general can make it easier to make sense of the code, especially for beginner users/contributors (in general the smaller the code base, the easier it is to attract contributors). Also having (brief) docs just for the solvers would be a very useful reference in general imho.
  • Efficient Poisson solvers are hard to write and it's an art of their own since different BCs generally require different algorithms. With that said, having a separate package just for that might not only benefit the community (a quick google search for something like "poisson solver julia" shows that there's interest) but also it might make it easier for someone who needs a specific algorithm that we don't yet have to make a contribution that might help us.

That said, I've never touched the Poisson solvers in Oceananigans so I don't know how "portable" they are, or if there are significant disadvantages. So some feedback here would be helpful.

Thoughts?

@christophernhill
Copy link
Member

@tomchor I think that could make sense. For FFT solver its pretty clean. The implicit free-surface solver knows more about grid and operators in certain places, which is more closely tied to other pieces of code.

@ali-ramadhan
Copy link
Member

I think this is a good idea. @glwagner actually started a package a long time ago: https://github.com/glwagner/StaggeredPoisson.jl

Some of the solvers uses some Oceananigans-specific types like Bounded and Periodic but otherwise the FFT-based solvers could easily be moved to a separate package. Just needs someone to put in the time and effort :P

@francispoulin
Copy link
Collaborator

I can imagine others benefiting from the Poisson solvers and wanting to use those without necessarily using Oceananigans, so I think this could work well.

I do have a question though. We have a lot of tests in Oceananigans now and that is a concern. If we had the solvers in a different package, what would be the savings? I guess we wouldn't have the solvers part of the test as that would be separate? From what I understand that's a small portion of all the tests, but I could be wrong.

@ali-ramadhan 's point is well taken, that it would need a champion to get this started.

@glwagner
Copy link
Member

glwagner commented Apr 7, 2021

I think the main downside of a separate package is that we have to maintain two CI pipelines, two docs, etc. The advantages are pretty clear though. It could make sense until there's some external interest in using the solvers in a standalone manner. Then we have both motivation and help to take on the work. The code is already modular with few circular dependencies so I don't think splitting up the repo would be difficult from a purely typing standpoint.

I think it's fine if the solvers are specific to Field and AbstractGrid. One could envision an ecosystem of packages for finite volume calculations on staggered grids:

  • StaggeredVolumes.jl or maybe StaggeredFields.jl (Oceananigans.Grids, Oceananigans.Operators, Oceananigans.Fields, Oceananigans.AbstractOperations)
  • StaggeredEllipticSolvers.jl (Oceananigans.Solvers) Note that these are both Poisson solvers and Helmholtz solvers.
  • We may also want to put the Simulation infastructure in a standalone package, because this could be used by any model (not just ocean-specific models)
  • Anything else?

Then Oceananigans.jl is mainly just the physics, models, and time-steppers for ocean problems.

@ali-ramadhan
Copy link
Member

I guess from a refactoring point of view it might also make sense to wait until we're closer to Oceananigans v1.0 before splitting off into multiple smaller packages. If we have to refactor code that involves deep changes (still likely) right now it might be easier to refactor 1 package rather than refactor across multiple packages + tag releases etc.

@francispoulin
Copy link
Collaborator

Very sound advice @ali-ramadhan and I vote for waiting until v1.0.

This does make me wonder what is required before we make it to 1.0? Is there a plan in the works?

@tomchor
Copy link
Collaborator Author

tomchor commented Apr 7, 2021

Waiting until closer to v1.0 sounds like a good plan. Although I have no idea how close we are to that goal.

I think the main downside of a separate package is that we have to maintain two CI pipelines, two docs, etc.

My idea about separating the Poisson solvers specifically was mainly motivated by my perception that that part of code the changes very rarely compared to the other parts. Thus the extra level of maintenance would be low. Although I might be wrong in my impression!

I also like the idea of separating other aspects of the code like @glwagner proposed. Although I don't have much of a feel of the work needed for those.

@glwagner
Copy link
Member

glwagner commented Apr 7, 2021

Although I don't have much of a feel of the work needed for those.

The typing is mostly copy/paste directories in Oceananigans.jl/src to another directory, defining what gets exported, and then changing import and using statements in Oceananigans correspondingly.

The more annoying part is setting up new repos with CI pipelines, docs, etc.

@glwagner
Copy link
Member

This is a great idea but nobody is working on it, so I'm closing the issue.

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

No branches or pull requests

5 participants