Allow loop_plugin for multi_output #357
Conversation
There was a problem hiding this comment.
In general I think it is fine, though I think this is one of the fundamental changes which should be covered with a test. Given that this PR is not urgent may I ask you to add one, when there is time. I can also check and add something when I have a free minute this week.
strax/plugin.py
Outdated
| # Convert from dict to array row: | ||
| for k, v in r.items(): | ||
| results[i][k] = v | ||
| if not self.multi_output: |
There was a problem hiding this comment.
Minor comment can we revert the logic, if self.multi_output: is a nano bit simpler.
There was a problem hiding this comment.
Sure, if it makes you but a nano bit more happy, I'm completely fine with it. Just though most people read from top to bottom and no one has been having self.multi_output issues .
|
Btw, you reached the next level solving problems by accident without need :D |
|
The tests are a good point, will put this in the refrigerator for a bit and make the tests (and undraft) |
…tion/strax into loop_plugin_multioutput
|
Am I missing something or is it really strange that the coverage does not increase after I just spend time on writing a unit test? |
What is the problem / what does the code in this PR do
Allow loop-plugins to be used with multiple outputs.
I tried to write a new loop plugin, but this yielded quite an incomprehensible traceback that the dtype was wrong. We should at least make a better error. So I just modified the loop plugin to just work in this case. There is some copy pasting but given it's only 10 lines, I don't care too much.
The only downside is that, while solving this, I realized I don't actually need it.
Can you briefly describe how it works?
Loop over assumed a single output, when a dict is returned for the different outputs, it got confused. Especially when setting up the results buffer.