146 function data steady label #149
Conversation
goalie/function_data.py
Outdated
| _labels = [] | ||
| for field_type in tp.field_types: | ||
| _labels.extend(self._label_dict[field_type]) | ||
|
|
||
| return AttrDict( | ||
| { | ||
| label: AttrDict( | ||
| {field: self._data_by_field[field][label] for field in tp.fields} | ||
| ) | ||
| for label in self.labels | ||
| for label in _labels |
There was a problem hiding this comment.
Instead of duplicating this, why not just implement it once in the __init__ method of the base class? The labels attribute that is computed there is never used with these changes. Why not just overwrite that definition with this new one? It works for me when I do that locally.
There was a problem hiding this comment.
Also, a list comprehension (rather than extend) would be more in line with the style used in Goalie.
There was a problem hiding this comment.
goalie/function_data.py
Outdated
| self.labels = ( | ||
| [ | ||
| [label for label in self._label_dict[field]] | ||
| for field in self.time_partition.field_types | ||
| ][0] | ||
| if self.time_partition.fields | ||
| else [] | ||
| ) |
There was a problem hiding this comment.
I'm really confused by this. Is it not sufficient to just do the following?
self.labels = self._label_dict["steady" if time_partition.steady else "unsteady"]
There was a problem hiding this comment.
@jwallwork23 - I had missed the steady attribute. Yes, that does significantly simplify things. See commit #32119bf
There was a problem hiding this comment.
Oh actually it's not quite so simple because (as @ddundo has pointed out previously) some fields might be "steady" and some might be "unsteady". So I don't think we can just set self.labels for all cases in this way. Instead, it should be determined per-field.
Apologies! So some refactoring will be required. Does what I said here sound right to you both?
There was a problem hiding this comment.
Ah yes, I should have also pointed that out in #116... sorry! That looks like it complicates _data_by_label in this mixed case, where we have both steady-unsteady fields, since unsteady fields will have labels that steady ones don't. Although, in this mixed case the "steady" fields aren't actually steady, so the notion of "forward_old" and "adjoint_next" still makes sense for them, right?
The reason why we introduced field types was to not get the no dependency error in MeshSeq._dependency. So I don't actually see a reason why we would care about field type anywhere else... so maybe we can get rid of this distinction in the solutions dictionaries here, and just extract "forward_old" and "adjoint_next" in a sensible way for these "steady" fields (i.e. not from dependencies).
There was a problem hiding this comment.
@ddundo - I think it would help to have a simple mixed case test. Do you have one already implemented elsewhere in the code base I can use as a go-by?
There was a problem hiding this comment.
It looks like in time_partition.py the field_types variable is set for all fields as unsteady as long as there is more than one subinterval or more than one timestep. If it is just one interval with one timestep, all fields are set to steady. That is, it looks like the field_types is automatically set to the same variable for all fields dependent on the time_partition.
As @ddundo pointed out, in a mixed case, there would be more than one timestep by default so even the steady field in a mixed case would need to be handled over all the timesteps. The additional unsteady labels forward_old and/or adjoint_next would still apply to the steady component of the mixed case to handle it over more than one timestep?
@jwallwork23 I am not sure that further refactoring is needed, given the current setup? I would like to first implement a mixed steady, unsteady case to see where the current setup fails.
There was a problem hiding this comment.
Thanks @acse-ej321, that would indeed be useful! I could probably simplify my ice evolution problem, but maybe someone at the meeting today will be able to propose a better example :)
There was a problem hiding this comment.
Okay I'm happy for this to be merged, noting that we should keep an eye out for whether an issue crops up when addressing #152.
Address issues in #146 by:
labelslist in bothdata_by_labelanddata_by_subfunctionfromself.time_partition.field_typesas was already done indata_by_field.steadycase in addition tounsteadycase.