Skip to content
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

Find or create an instrument without worrying about existing instances #1210

Merged
merged 10 commits into from
Aug 16, 2018

Conversation

astafan8
Copy link
Contributor

@astafan8 astafan8 commented Jul 26, 2018

This new function find_or_create_instrument avoids the hassle of usage of Instrument.find_instrument. In case an instrument does not already exist, the latter will throw an exception, while the new function will just return the reference to that instrument.

If this function is used to create instruments, then it can be reexecuted, because unlike find_instrument or just reinstantiation with a constructor, this function does not throw. Moreover, thanks to the recreate kwarg, it allows to recreate an instance of the instrument in case one already exists.

Researchers find this convenience function very handy. pytopo repository (Delft lab) also has a variation of this convenience function. Even I ended up implementing this function for v0_characterization experiment and repository.

@wpfff Let me know if this is good enough (I've decided to avoid the force_new_instance feature). Update: aligned on the solution.

It avoids the need to worry about which instruments are instantiated
and which are not, and conveniently works out possible exceptions
raised from the Instrument.find_instrument method.
@codecov
Copy link

codecov bot commented Jul 26, 2018

Codecov Report

Merging #1210 into master will increase coverage by 0.23%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
+ Coverage   80.53%   80.76%   +0.23%     
==========================================
  Files          49       49              
  Lines        6821     6841      +20     
==========================================
+ Hits         5493     5525      +32     
+ Misses       1328     1316      -12

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could consider documenting and or testing what happens if an instrument of the same name but a different class exists. As far a I can see that will raise a TypeError inside find_instrument

Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@astafan8
Copy link
Contributor Author

just added a test and a note about "same name, different class" case. If necessary, the current behavior can be revisited later.

@astafan8 astafan8 merged commit 1470658 into microsoft:master Aug 16, 2018
@astafan8 astafan8 deleted the feature/instrument-factory branch August 16, 2018 14:43
giulioungaretti pushed a commit that referenced this pull request Aug 16, 2018
Merge: 700e0cb b7266ca
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1210 from astafan8/feature/instrument-factory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants