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

Chamel/equinox api #87

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Chamel/equinox api #87

wants to merge 23 commits into from

Conversation

cmhamel
Copy link
Collaborator

@cmhamel cmhamel commented Jun 28, 2024

Attempt at "re-writing part of the interface as equinox modules. This buys us a few things

  1. Allows for jittable containers for Functionspaces, Mechanics, etc.
  2. Easier more python like coding style with class structures, methods, and inheritance
  3. Reduces code bloat by removing a functional style that jax enforces
  4. Probably some other things I haven't thought of yet.

I've mainly ported QuadratureRule, FunctionSpace, and Mechanics so far.

Also I created a new class called "Domain" which removes a good bit of boiler plate code to run an optimism problem.

@cmhamel cmhamel marked this pull request as draft June 28, 2024 17:47
@cmhamel
Copy link
Collaborator Author

cmhamel commented Jun 28, 2024

The vast majority of file changes in this PR are handled in my other PR related to unit testing and code coverage. This was branched off of that branch

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 79.11548% with 85 lines in your changes missing coverage. Please review.

Project coverage is 76.37%. Comparing base (6782079) to head (4454bac).

Files with missing lines Patch % Lines
optimism/Mechanics.py 83.03% 38 Missing ⚠️
optimism/Problem.py 41.46% 24 Missing ⚠️
optimism/Domain.py 63.93% 22 Missing ⚠️
optimism/Mesh.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   76.40%   76.37%   -0.03%     
==========================================
  Files          61       64       +3     
  Lines        5051     5227     +176     
==========================================
+ Hits         3859     3992     +133     
- Misses       1192     1235      +43     

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

@cmhamel cmhamel marked this pull request as ready for review June 29, 2024 00:49
@cmhamel cmhamel changed the title [WIP] Chamel/equinox api [DO NOT MERGE] Chamel/equinox api Jun 30, 2024
@cmhamel cmhamel requested review from ralberd, tupek2 and btalamini July 8, 2024 15:39
@cmhamel
Copy link
Collaborator Author

cmhamel commented Jul 8, 2024

This is a start. Mechanics.py could be cleaned up a bit more but the main question is should we make multiblock the standard rather than having two different implementations floating around.

@btalamini
Copy link
Collaborator

This is a start. Mechanics.py could be cleaned up a bit more but the main question is should we make multiblock the standard rather than having two different implementations floating around.

I’m looking forward to test driving this!

I’m also bugged about the code duplication with the multi-block stuff. I was considering going the other way and removing the multi-block version. There are usually only a few materials in our simulations. It might be easier to just have the user sum the contributions in their energy function.

@ralberd
Copy link
Contributor

ralberd commented Aug 29, 2024

This is a start. Mechanics.py could be cleaned up a bit more but the main question is should we make multiblock the standard rather than having two different implementations floating around.

I’m looking forward to test driving this!

I’m also bugged about the code duplication with the multi-block stuff. I was considering going the other way and removing the multi-block version. There are usually only a few materials in our simulations. It might be easier to just have the user sum the contributions in their energy function.

I agree with Brandon, it may be cleaner to just have the single block mechanics functions and then the Domain class that you added can handle looping over blocks to compute energies, etc. I think it makes sense for Domain to contain the knowledge of blocks.

Copy link
Contributor

@ralberd ralberd left a comment

Choose a reason for hiding this comment

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

This looks pretty good. Once you address the comments and the TODO items it should be good to go.

Comment on lines +57 to +58
def get_bc_values(self, U):
return self.dof.get_bc_values(U)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this function necessary? It looks like get_ubcs covers this functionality.


def update_field(self, Uu, p):
Ubc = self.get_ubcs(p)
return self.create_field(Uu, Ubc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be return self.dof.create_field(Uu, Ubc)? Then you could get rid of self.create_field

def create_field(self, Uu, Ubc):
return self.dof.create_field(Uu, Ubc)

def create_unknowns(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def create_unknowns(self):
def initialize_unknowns(self):

Uu = self.dof.get_unknown_values(np.zeros(self.mesh.coords.shape))
return Uu

def energy_function(self, Uu, p):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for this to be extended easily to incorporate things like traction energy? Or would a different Domain-like class be required for each energy type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is the one place where we'll need to do some functional programming under the hood. We initializing the Domain we can call some called "create_energy_function" that will construct things appropriately depending upon what inputs are provided to the constructor.

shapeGrads: Float[Array, "ne nqpe npe nd"]
mesh: any
quadratureRule: QuadratureRule.QuadratureRule
isAxisymmetric: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea to eventually move the contents of construct_function_space into the constructor for this class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah exactly. I was going to wait to do that until a future PR since, otherwise it would've changed a large number of test files.

@@ -64,8 +83,8 @@ def construct_structured_mesh(Nx, Ny, xExtent, yExtent, elementOrder=1, useBubbl
return mesh


def get_blocks(mesh, blockNames):
return tuple(mesh.blocks[name] for name in blockNames)
# def get_blocks(mesh, blockNames):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this no longer used?

optimism/Objective.py Outdated Show resolved Hide resolved
optimism/Problem.py Outdated Show resolved Hide resolved
optimism/Problem.py Outdated Show resolved Hide resolved
optimism/Problem.py Outdated Show resolved Hide resolved
@ralberd
Copy link
Contributor

ralberd commented Aug 29, 2024

@btalamini @tupek2. Once this gets merged in, what should be the plan to deal with the old functions? How important is backwards compatibility?

cmhamel and others added 17 commits September 11, 2024 17:25
…the functionspace level and created a class structure for mechanics that we can now derive off while jitting and vmapping and gradding.
…k class could be removed eventually. Had to switch from jit to filter_jit in a few places in tests, but minor changes.
…greatly pairs down the effort to setup a new simulation.
…minor change to the json mesh reader and a test.
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
cmhamel and others added 6 commits September 11, 2024 17:25
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
Co-authored-by: ralberd <[email protected]>
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.

4 participants