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

ENH:Questionnaire Motor Guess #51

Merged
merged 6 commits into from
Mar 2, 2018
Merged

ENH:Questionnaire Motor Guess #51

merged 6 commits into from
Mar 2, 2018

Conversation

teddyrendahl
Copy link
Contributor

DO NOT MERGE UNTIL pcdshub/pcdsdevices#167

Description

When loading motors from the Questionnaire, guess which type each is by looking at the prefix. I view this is a temporary fix until the QuestionnaireClient can tell us explicitly which class should be loaded.

Question: Should the default be pcdsdevices.epics_motor.PCDSMotorBase or ophyd.EpicsMotor? Not sure ... My only thought is that our base version will work on true MotorRecord motors, but the EpicsMotor won't work with our bastards.

Motivation and Context

Without this fix Newport and PMC100 motors will not load properly from the Questionnaire.

How Has This Been Tested?

Added tests for both guessing the motor type, and that it works in situ.

Type of EpicsMotor. If not, we assume can use pcdsd
"""
for _typ in motor_types:
if _typ in prefix:
Copy link
Member

Choose a reason for hiding this comment

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

Might want to be more robust than this, just in case a motor comes through with a wonky name like XPP:USR:MZM:AMMN... contrived example, I know. I suggest checking prefix.split(':') and trying to match a piece of it.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, forget about this, we have to strip this out when the questionnaire gets updated anyway.

@ZLLentz
Copy link
Member

ZLLentz commented Mar 2, 2018

Just make sure you take care of the flake8 issue that travis is mad about 👍

@codecov-io
Copy link

codecov-io commented Mar 2, 2018

Codecov Report

Merging #51 into master will decrease coverage by 0.06%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   89.86%   89.79%   -0.07%     
==========================================
  Files          11       11              
  Lines         592      598       +6     
==========================================
+ Hits          532      537       +5     
- Misses         60       61       +1
Impacted Files Coverage Δ
happi/backends/qs_db.py 69.56% <75%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 139ad0c...df03919. Read the comment docs.

@teddyrendahl
Copy link
Contributor Author

Okay one more thought. This will break for anything older than the current pcdsdevices. These classes were just introduced but never actually imported. How do I express this implicit dependency?

@ZLLentz
Copy link
Member

ZLLentz commented Mar 2, 2018

I think if we've committed to making this dependency implicit, there's nothing to explicitly do about it. Libraries that need an older pcdsdevices must also specify an older happi.

@teddyrendahl teddyrendahl merged commit 443ff9b into pcdshub:master Mar 2, 2018
@teddyrendahl teddyrendahl deleted the motor_guess branch March 2, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants