-
Notifications
You must be signed in to change notification settings - Fork 301
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
Feature: Added instrument paramater snapshot_exclude #1653
Feature: Added instrument paramater snapshot_exclude #1653
Conversation
Note that I still need to add tests. |
Codecov Report
@@ Coverage Diff @@
## master #1653 +/- ##
==========================================
+ Coverage 67.07% 67.08% +<.01%
==========================================
Files 144 144
Lines 17679 17686 +7
==========================================
+ Hits 11858 11864 +6
- Misses 5821 5822 +1 |
I've added tests and also tested the new implementation on a device on my desk. Can someone review and merge the code? @jenshnielsen, @WilliamHPNielsen? |
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, I left a few comments inline. In addition it seems like some of the lines are more than 80 char l Could you check this
I've adapted the code after review and fixed the line length issues. |
Looks good. I will leave this for a bit to givee others a chance to merge |
Changes proposed in this pull request:
Note that this is very similar to #1651, the goal is the same but it has a different implementation. To see the reason why we need this please look at #1651 which has a detailed problem description. This implementation was suggested by @jenshnielsen, which uses the instrument parameter to store the
snapshot_exclude
instead of providing a list to the snapshot function.@jenshnielsen, @AdriaanRol