107 mixed space adj project #108
Conversation
There was a problem hiding this comment.
Thanks for this. Seems the right thing to do.
One request and then we'll need to merge #104 to get fixes that are currently holding up the test suite.
| @@ -61,8 +62,12 @@ def get_enriched_mesh_seq( | |||
| get_qoi=self._get_qoi, | |||
| get_bcs=self._get_bcs, | |||
| qoi_type=self.qoi_type, | |||
| parameters=self.params, | |||
There was a problem hiding this comment.
Thanks. Partially addresses #97.
goalie/go_mesh_seq.py
Outdated
| # update self._fs variable for enriched mesh | ||
| mesh_seq_e.function_spaces |
There was a problem hiding this comment.
Please could you introduce a method on the MeshSeq to do this whose name says what it is doing, e.g., update_function_spaces? On it's own, this line of code looks like it's doing nothing. (My bad for writing it this way in the first place, I was probably overusing properties.)
There was a problem hiding this comment.
@jwallwork23
I added a function called update_function_spaces which calls function_spaces if the function space attribute dictionary is None. Hopefully, that is a bit more readable.
|
@acse-ej321 Apologies for pushing to your branch but I thought it would be easier than explaining my suggestion. What do you think of these changes?
|
goalie/go_mesh_seq.py
Outdated
|
|
||
| # Update function spaces for enriched mesh | ||
| mesh_seq_e.update_function_spaces |
There was a problem hiding this comment.
Note that I prefer not to add comments that are already clear from the code / method name.
There was a problem hiding this comment.
I'm writing some Development Practices over in the Movement wiki and I'll put this in there.
There was a problem hiding this comment.
@jwallwork23 - Noted. I have a tendency to over-comment and will work on it especially when the attempt is to make the method name self explanatory.
| @@ -234,8 +235,10 @@ def _function_spaces_consistent(self) -> bool: | |||
| ) | |||
| return consistent | |||
|
|
|||
| @property | |||
| def function_spaces(self) -> list: | |||
| def update_function_spaces(self) -> list: | |||
There was a problem hiding this comment.
@jwallwork23 - I think your edits here make more sense than my interpretation of your suggestions and it is a better descriptive name of the method you had already implemented.
|
@acse-ej321 Are you happy to merge this? It's good to go. (Usually I leave merging to the developer.) |
fixes #107
Calls the override
projectfunction instead of the default Firedrake function to deal with mixed function spaces and adjoints.Additionally, modifies the creation of the enriched mesh to:
self._fsvariable required for mesh-to-mesh conservative transfer