Clear fields and meshes after indicate error calculated per segment #224
Conversation
acse-ej321
changed the title
|
Thanks for this @acse-ej321! And yeah, sorry... I should have done that separately. Let me open a PR now to fix those and then you can merge it into this branch :) |
… 220_clear_fields_after_indicate merge #222 update caching fix into branch
goalie/go_mesh_seq.py
Outdated
| enriched_mesh_seq.time_partition.num_subintervals -= 1 | ||
| enriched_mesh_seq.time_partition.timesteps.pop() |
There was a problem hiding this comment.
It'd be good to add a method to TimePartition to properly drop the last subinterval and use that here.
There was a problem hiding this comment.
see commit 8e78325
goalie/go_mesh_seq.py
Outdated
| self.solutions[f][FWD_OLD].pop() | ||
| self.solutions[f][ADJ_NEXT].pop() |
There was a problem hiding this comment.
Is there a situation in which we would not want to destroy these? If so, maybe add a kwarg to not destroy them?
And if we do destroy them, I think you should modify the labels in self.solutions at the end of GoalOrientedMeshSeq.indicate_errors, so that we don't run into errors trying to access data corresponding to those labels.
There was a problem hiding this comment.
@dundo - At the moment, after indicate errors runs through all subintervals, self.solutions still has a key for all default labels just with empty lists as associated values. Before destroying the label keys in self.solutions, can you think of a case where they might be reused after running indicate errors?
There was a problem hiding this comment.
I can't think of something like that, no - in the Hessian adaptation either. But I might be missing something, so I asked that question in the issue #220 :)
goalie/go_mesh_seq.py
Outdated
| enriched_mesh_seq.solutions[f][ADJ_NEXT].pop() | ||
|
|
||
| # delete extra mesh to reduce the memory footprint | ||
| enriched_mesh_seq.meshes.pop() |
There was a problem hiding this comment.
I think you also need to remove function spaces in enriched_mesh_seq._fs in order to fully get rid of the mesh. Maybe somewhere else too
There was a problem hiding this comment.
see commit 714f9ef - added call to update_function_spaces which uses the existing MeshSeq method to rebuild enriched_mesh_seq._fs based on the updated subintervals.
ace after removing mesh subinterval
last interval
| def drop_last_subinterval(self): | ||
| """ | ||
| Drop the last subinterval and reset lists and counters appropriately. | ||
| """ | ||
| self.end_time = self.subintervals[-1][0] | ||
| self.interval = (self.start_time, self.end_time) | ||
| self.num_subintervals -= 1 | ||
| self.timesteps.pop(-1) | ||
| self.subintervals.pop(-1) | ||
| self.num_timesteps_per_subinterval.pop(-1) | ||
| self.num_timesteps_per_export.pop(-1) | ||
| self.num_exports_per_subinterval.pop(-1) |
There was a problem hiding this comment.
Would it be better to modify these arguments that we pass to __init__ and then reinitialise it with self.__init__(*modified_args)?
There was a problem hiding this comment.
And sorry, why do we actually need to modify the time partition? :)
There was a problem hiding this comment.
@ dundo - _function_spaces_consistent checks whether there is a time partition for each subinterval. The modification to time_partition was to comply with that requirement.
| # delete current subinterval enriched mesh to reduce the memory footprint | ||
| enriched_mesh_seq.meshes.pop(-1) | ||
| enriched_mesh_seq.time_partition.drop_last_subinterval() | ||
| enriched_mesh_seq._update_function_spaces() |
There was a problem hiding this comment.
Hmm I'm not sure this would work for p-enrichment (see the last lines of get_enriched_mesh_seq above).
But I think it's also unnecessary extra effort to rebuild all i-1 function spaces since we just want to get rid of the last one :)
| def drop_last_subinterval(self): | ||
| """ | ||
| Drop the last subinterval and reset lists and counters appropriately. | ||
| """ | ||
| self.end_time = self.subintervals[-1][0] | ||
| self.interval = (self.start_time, self.end_time) | ||
| self.num_subintervals -= 1 | ||
| self.timesteps.pop(-1) | ||
| self.subintervals.pop(-1) | ||
| self.num_timesteps_per_subinterval.pop(-1) | ||
| self.num_timesteps_per_export.pop(-1) | ||
| self.num_exports_per_subinterval.pop(-1) |
There was a problem hiding this comment.
When adding a new method please implement unit tests.
| # clear empty labels | ||
| for f in self.fields: | ||
| if self.steady: | ||
| self.solutions.labels = "forward" |
There was a problem hiding this comment.
Does this need to be inside a tuple?
|
@acse-ej321 if you want, we can keep this PR simpler and just delete the fields. Then we can deal with labels in a separate PR together with #226? |
|
As we discussed in the meeting, I guess that doing some garbage collection might help after we remove these fields and meshes by using the in-built |
Closes #220
indicate_errorsto remove extra fields for the base and enrichedsolutionsfields after each subinterval iteration.enriched_mesh_seqmesh after each interaction and resets class variables accordingly.get_enriched_mesh_seqto pass a deepcopy ofTimePartitionto the enriched mesh sequence constructor to allow for deleting a mesh subinterval and correctly reindexing.