Meshes are now iterable #295
Conversation
| """ | ||
| # TODO #122: Refactor to use the set method | ||
| if not isinstance(meshes, Iterable): | ||
| if not isinstance(meshes, list): |
There was a problem hiding this comment.
How about we allow tuple to cover more possible cases?
| if not isinstance(meshes, list): | |
| if not isinstance(meshes, (list, tuple)): |
There was a problem hiding this comment.
I committed this but I'm not sure we should allow tuples. In adaptor functions in demos we have mesh_seq[i] = adapt(mesh_seq[i], metric), so we need a mutable object to be able to reassign meshes, right?
There was a problem hiding this comment.
I tested this and we indeed get an error:
File "/home/firedrake/goalie/demos/burgers-hessian.py", line 145, in adaptor
mesh_seq[i] = adapt(mesh_seq[i], metric)
~~~~~~~~^^^
File "/home/firedrake/goalie/goalie/mesh_seq.py", line 130, in __setitem__
self.meshes[subinterval] = mesh
~~~~~~~~~~~^^^^^^^^^^^^^
TypeError: 'tuple' object does not support item assignment
So I reverted this. Am I missing something? Sorry if I am!
There was a problem hiding this comment.
Ah yes, I always forget about this. Thanks.
ddundo
force-pushed
the
fix_iterable_mesh
branch
from
f23b790 to
90ae934
Compare
|
Thanks @jwallwork23. Please see the comment. Also had to fix a new linting error |
| if len(adjoint_error.keys()) != 1: | ||
| raise ValueError("Inconsistent input for 'adjoint_error' and 'test_space'.") | ||
| test_space = {key: test_space for key in adjoint_error} | ||
| test_space = dict.fromkeys(adjoint_error, test_space) |
There was a problem hiding this comment.
Thanks for fixing this. I spotted it in the weekly CI outputs but hadn't got round to it.
| """ | ||
| # TODO #122: Refactor to use the set method | ||
| if not isinstance(meshes, Iterable): | ||
| if not isinstance(meshes, list): |
There was a problem hiding this comment.
Ah yes, I always forget about this. Thanks.
The CI in #291 fails since meshes are now iterable (not our change). So in this PR I just changed
isinstance(meshes, Iterable)toisinstance(meshes, list).This will anyway get refactored in #239 to always require a list of meshes, since
will not be possible to do since
MeshSeqwill be completely separated fromTimePartitions, so there won't be aself.subintervalsattribute anymore.