Resolve "Operation Splitting"
Closes #6 (closed)
Merge request reports
Activity
changed milestone to %Period 2 Kanban
added Needs testing + 1 deleted label
237 237 def split(self) -> Iterable[Operation]: 238 return self.operations 238 """ Returns every operation in the SFG except for Input and Output types. """ 239 ops = [] 240 for op in self.operations: 241 if not isinstance(op, Input) and not isinstance(op, Output): 242 ops.append(op) 243 244 return ops # Need any checking before returning? 245 246 247 def replace_self(self) -> None: 248 """ Iterates over the SFG's (self) Input- and OutputSignals to reconnect them to each necessary operation inside the SFG, 249 so that the inner operations of the SFG can function on their own, effectively replacing the SFG. """ 250 251 assert len(self.inputs) == len(self.input_operations), "Number of inputs does not match the number of input_operations in SFG." changed this line in version 5 of the diff
214 215 add1.input(0).connect(inp1, "S1") 216 add1.input(1).connect(inp2, "S2") 217 add2.input(0).connect(add1, "S3") 218 add2.input(1).connect(inp3, "S4") 219 mul1.input(0).connect(add1, "S5") 220 mul1.input(1).connect(add2, "S6") 221 out1.input(0).connect(mul1, "S7") 222 223 mac_sfg = SFG(inputs = [inp1, inp2], outputs = [out1]) 224 225 inp4 = Input("INP4") 226 inp5 = Input("INP5") 227 out2 = Output(None, "OUT2") 228 229 mac_sfg.inputs[0].connect(inp4, "S8") changed this line in version 5 of the diff
235 235 return value 236 236 237 237 def split(self) -> Iterable[Operation]: 238 return self.operations 238 """ Returns every operation in the SFG except for Input and Output types. """ 239 ops = [] 240 for op in self.operations: 241 if not isinstance(op, Input) and not isinstance(op, Output): 242 ops.append(op) 243 244 return ops # Need any checking before returning? 245 246 247 def replace_self(self) -> None: -
The name and usage of this method is very confusing to me. If you just read the function signature in plain English, it just says "replace self" and that it takes nothing and returns nothing. Replace yourself with what? What is the expected result? If I just have a random SFG lying around and I tell it to "replace itself", what should I expect to happen? Even reading the docstring, I can't fully grasp it in a way that makes sense. There definitely has to be a better way of doing this.
changed this line in version 5 of the diff
added Needs review label and removed Needs testing label
added 1 commit
- b3b2d21c - replace_self() -> replace_sfg_with_inner_components. Changed docstring....
234 234 results[self.key(index, prefix)] = value 235 235 return value 236 236 237 def replace_sfg_with_inner_components(self) -> bool: -
I don't like how this method essentially leaves the SFG variable in an invalid state after being called. It might not be obvious to users that the SFG should not be used again after calling this method. I also think this means that the name still does not accurately describe what really happens. The SFG is not replaced with anything, it still exists in the same variable after the call, it has just been invalidated. Perhaps a more accurate name would be something like "connect_external_signals_to_internal_operations". However, considering how long and convoluted the name needs to be to describe it, it may still be better to come up with a different way of accomplishing this altogether.
changed this line in version 7 of the diff
242 if len(self.outputs) != len(self.output_operations): 243 raise IndexError(f"Number of outputs does not match the number of output_operations SFG.") 244 245 try: 246 # For each input_signal, connect it to the corresponding operation 247 for port, input_operation in zip(self.inputs, self.input_operations): 248 dest = input_operation.output(0).signals[0].destination 249 dest.clear() 250 port.signals[0].set_destination(dest) 251 # For each output_signal, connect it to the corresponding operation 252 for port, output_operation in zip(self.outputs, self.output_operations): 253 src = output_operation.input(0).signals[0].source 254 src.clear() 255 port.signals[0].set_source(src) 256 return True 257 except IndexError: -
Exceptions should be reserved for exceptional cases (hence the name). Using them for control flow is usually considered bad practice. In this case, considering the returned bool is an expected return value of the function, it seems strange to have its value originate from an exception caused by purposefully violating the contracts of other functions. This implementation also relies on the exact exception type raised being an IndexError. This could be replaced with if-statements that check the condition that would cause the exception to arise and return False early instead.
Edited by Ivar Härnqvist changed this line in version 7 of the diff
added 1 commit
- 21c5a392 - Fixed threads about comments/newlines/function_name/exceptions.
