-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Patched a potential segfault when appending simulations #803
Patched a potential segfault when appending simulations #803
Conversation
… data is problematic, adding now a user warning.
Hi @VictorSeven, thanks for making this PR and sorry for the delayed response. I suggest to just check whether At the moment the tests are failing at the Best regards, and thanks again for looking into this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
see comment in PR. tests are failing because of black
formatting. See https://github.com/mackelab/sbi/blob/main/CONTRIBUTING.md#style-conventions for how to run formatting checks locally.
Hello, sorry for the delay -was a bit sick last weeks. I have updated the change as suggested to use num_unique instead. I run About use case: I understand the confusion about the use case of training with a single data point. In my case, my dynamical system was always subject to a single external input, which I was not passing to SBI, because it was always the same. Then, I wanted to train SBI to recognise several different external inputs. As a test, I rewrote the code where now SBI was aware of the external input, and I was to check if giving it only 1 would yield the same results as I had before. I understand it was maybe not intended usage and this led to an error, but the main problem was that the Python kernel would just silently crash, making it difficult to debug the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks!
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #803 +/- ##
==========================================
+ Coverage 74.76% 74.85% +0.08%
==========================================
Files 80 80
Lines 6190 6199 +9
==========================================
+ Hits 4628 4640 +12
+ Misses 1562 1559 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 22 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I had some problems when calling the ``append_simulations` method. In a Jupyter Notebook, it would kill the kernel every time without further notice. In a script, one can see this is due to a segmentation fault.
After some debugging, I spotted the problem when checking the Z-score of the data. In my case, the result of the simulation was two
vstacked
time series (external input and simulated time series; the tensor shape was(batch_size, 2, n_data)
). For testing purposes, I was training the SBI only with a single realization of the external input, thus leading to 0 variance among all batches.Despite this not being a very common use case, the fact that it kills the Python kernel makes the problem difficult to track for the user: there is no error message and the debugger does not work properly (it will just crash, not going inside the SBI code). Even
print
statements inside the functionappend_simulations
would not print out in a notebook, being visible only when used on a script.For this reason, I added a small extra check in the code. If this edge case arises, the computation of the Z-score is skipped, and now the user is adequately warned about the problem so it can be fixed when needed. From what I have seen code would run afterwards without problems.