Window placement handling#448
Conversation
wbenoit26
left a comment
There was a problem hiding this comment.
Overall I quite like the idea of making it obvious where the window will be selected from. One thing I'm not clear on: does this enable new functionality, or is it just a re-parameterization that you find more intuitive?
| # preprocessing args | ||
| # kernel_length: | ||
| windowing: | ||
| # kernel_length: |
There was a problem hiding this comment.
The line here needs to be updated so that the train config inherits the shared kernel length.
| return self.window_lead_max - self.window_lead_min | ||
|
|
||
| def __str__(self) -> str: | ||
| width = 60 |
There was a problem hiding this comment.
Any particular reason for this width? I'd imagine you could double it without running into wrapping issues on most screen sizes.
There was a problem hiding this comment.
addressed in fb03f5b (will start a new PR since this PR is directed to the wrong base branch and fork)
@benedict-armstrong could you give an example what error you see? I personally find left_pad and right_pad pretty intuitive as opposed to leading_edge. So I have the same thought as @wbenoit26 in #448 (review). |
The current
left_padandright_padare a bit complicated and error prone. This PR introduces a new more explicit way to set the window the model sees and adds a logging statement to avoid errors.For example the following config:
prints:
For debugging and to avoid errors this is logged in the dataloader setup.
The change also allows pre merger windows like:
which prints:
This PR also updates the existing configs to work in this format. Possible extensions would be to add a logging statement for the location of the validation waveform views.