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

Implements MPCs #108

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Implements MPCs #108

wants to merge 6 commits into from

Conversation

ralberd
Copy link
Contributor

@ralberd ralberd commented Jan 23, 2025

This is a work in progress to implement MPCs.

@ralberd ralberd requested a review from cmhamel January 23, 2025 00:11
@ralberd ralberd self-assigned this Jan 23, 2025
@ralberd ralberd marked this pull request as draft January 23, 2025 00:11
@ralberd ralberd removed their assignment Jan 23, 2025
@ralberd
Copy link
Contributor Author

ralberd commented Jan 23, 2025

@SarveshJoshi33 Thanks for making a fork so we can start to review this work. I opened a Pull Request so we can compare changes and make comments here.

@ralberd ralberd self-assigned this Jan 23, 2025
Comment on lines 366 to 379
class DofManager(eqx.Module):
# TODO get type hints below correct
# TODO this one could be moved to jax types if we move towards
# TODO jit safe preconditioners/solvers
fieldShape: Tuple[int, int]
isBc: any
isUnknown: any
ids: any
unknownIndices: any
bcIndices: any
dofToUnknown: any
HessRowCoords: any
HessColCoords: any
hessian_bc_mask: any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like you started making changes to FunctionSpace.py before some newer changes were made to make it an Equinox module. Can you keep the Equinox module implementations of FunctionSpace and DofManager classes and implement your DOFManagerMPC class as a module? Let me know if you want my help.

Choose a reason for hiding this comment

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

Hi Ryan,
I updated the DofManagerMPC class using the updated FunctionSpace with the equinox module and pushed it to the forked repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like you made changes to test files before they got moved out of the test directory. Could you move these back to where they are in the latest main branch? That way we can see clearly any changes you made. Again, let me know if you want help.

Copy link
Contributor Author

@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.

@cmhamel I wonder if there's a way to inherit from the existing DofManager class to cut down on code repetition.

@@ -466,3 +490,181 @@ def _make_hessian_bc_mask(self, conns):
hessian_bc_mask[e,eFlag,:] = False
hessian_bc_mask[e,:,eFlag] = False
return hessian_bc_mask


# Different class for Multi-Point Constrained Problem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
# Different class for Multi-Point Constrained Problem
# DOF Manager for Multi-Point Constrained Problem

Comment on lines 496 to 499
from optimism import Mesh
import equinox as eqx
import jax.numpy as np
import numpy as onp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
from optimism import Mesh
import equinox as eqx
import jax.numpy as np
import numpy as onp

These can all be removed as they are imported at the top of the file.

import equinox as eqx
import jax.numpy as np
import numpy as onp
from typing import List, Tuple
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
from typing import List, Tuple

You can just change line 6 to this.

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