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

Small refactor to remove inverted dependencies in Albany #579

Closed
bartgol opened this issue Apr 27, 2020 · 12 comments · Fixed by #584
Closed

Small refactor to remove inverted dependencies in Albany #579

bartgol opened this issue Apr 27, 2020 · 12 comments · Fixed by #584
Assignees

Comments

@bartgol
Copy link
Collaborator

bartgol commented Apr 27, 2020

I thought about the issue of circular dependencies in Albany. The main issue is with the various factories (or factory-like) classes in Albany, which require knowledge about the subpackages.

After thinking about it, I think we should do something like this:

  1. For each factory type XYZ (e.g., ProblemFactory), create a base abstract class (e.g. ProblemFactoryBase), and have only the "generic" Albany objects created in the 'default' albany factory.
  2. Packages can have their own factory, which inherits from the base one.
  3. Most of the creation happens in the application. So let's make the factories 'settable' in the application, with the default one always present.
  4. In the Main_.cpp file we add all factories, including LandIce ones. This is ok, since Main_.cpp is not in AlbanyLib, so it can depend on LandIce too.
  5. Similar to 4, the mpas interface has to set LandIce factories in the application
  6. Probably need to modify a tad also the SolverFactory (which is probably good anyways, especially since we pruned some stuff and maybe the solver factory can be cleaned a bit).

@mperego @gahansen @ikalash @jewatkins I'm planning to do something on this side, if you're ok with it. Should be pretty quick.

@bartgol bartgol self-assigned this Apr 27, 2020
@jewatkins
Copy link
Collaborator

This sounds good to me.

@mperego
Copy link
Collaborator

mperego commented Apr 27, 2020

Yes, it would be nice to have the Package (even if now it's only one) separated from the core files.. if it doesn't take long.

@ikalash
Copy link
Collaborator

ikalash commented Apr 27, 2020

I'm ok with these changes.

bartgol added a commit that referenced this issue Apr 28, 2020
Contributes to remove dependence of main albany on packages (see #579).
@gahansen
Copy link
Member

I'm also OK with this - sounds like the right way to go.

@bartgol
Copy link
Collaborator Author

bartgol commented Apr 28, 2020

Can we make STK a required dependency of Albany? Now that SCOREC is out of the picture, STK meshes are the only option...

@gahansen
Copy link
Member

I vote yes.

@jewatkins
Copy link
Collaborator

Seems okay to me. It shouldn't be too hard to make it optional again if we decide to introduce some other discretization down the line. Or were you thinking of getting rid of the entire discretization abstraction?

@bartgol
Copy link
Collaborator Author

bartgol commented Apr 28, 2020

No, I'll leave the abstraction, with the factory and everything. But since we only have STK, it seemed silly to make it 'optional'.

bartgol added a commit that referenced this issue Apr 29, 2020
Contributes to remove dependence of main albany on packages (see #579).
@gahansen
Copy link
Member

In conversations with @bartgol, we decided to remove STK Adapt, Rebalance, and Percept as part of this small refactor. As such, this issue replaces and supercedes issue #37 - I am closing the earlier issue.

@bartgol
Copy link
Collaborator Author

bartgol commented Apr 29, 2020

@gahansen I ended up pruning the stk_adapt stuff, but kept the stk rebalance stuff. I believe we are using it together with zoltan when we need to redistribute the mesh, which, I think happens for instance when we load a stk mesh serially, and distribute it among ranks on the fly.

In #37 Dan said the rebalance stuff is supported directly in SEACAS. I think for now we can keep the snapshot of the old stk_rebalance in Albany (which I think works, at least for the serial mesh thingy). When time permits, we should see if we can directly use SEACAS, and purge the percept directory completely.

@gahansen
Copy link
Member

@bartgol sounds great to me. I'm familiar with the stk rebalance logic, I'll volunteer to do what @ibaned suggests - I'll open a new issue to replace the current approach with Ioss. I want to look around in Ioss again anyway to see what I can use for block data support these days.

@bartgol
Copy link
Collaborator Author

bartgol commented Apr 29, 2020

To be clear, I think this is not a priority for now. We should probably focus on the block discretization stuff first.

bartgol added a commit that referenced this issue May 4, 2020
Contributes to remove dependence of main albany on packages (see #579).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants