MNT: BITS structure cleanup #47
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MDecarabas
added
documentation
MDecarabas
force-pushed
the
switch_case_organization
branch
from
446d73a to
01d880a
Compare
|
@prjemian What is the advantage of having a factory for the sim_motor and sim_det? It looks like a convoluted way for something very simple. Is there a scenario where it will actually be used in the future? |
|
I think I understand but it feels like the naming is a bit off.
Maybe something like, "sim_motor_device" and "sim_detector_device" could be
more appropriate, factory implies the creation of many.
@eric Codrea ***@***.***> you are moving this file around right?
What is your suggestion?
…On Fri, Mar 7, 2025 at 12:39 PM Pete R Jemian ***@***.***> wrote:
The ophyd simulators support the default three plans that test an initial
installation works as expeceted. As described in previous issues (such as
#32 <#32> and #40
<#40>), the ophyd simulators were
the last remaining objects that could not be described in devices.yml.
The factory method makes that possible.
—
Reply to this email directly, view it on GitHub
<#47 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFMPQZSP2HGDITJIHO2VOT2THRWTAVCNFSM6AAAAABYPORJC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBXGE2TSNJUGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: prjemian]*prjemian* left a comment (BCDA-APS/BITS#47)
<#47 (comment)>
The ophyd simulators support the default three plans that test an initial
installation works as expeceted. As described in previous issues (such as
#32 <#32> and #40
<#40>), the ophyd simulators were
the last remaining objects that could not be described in devices.yml.
The factory method makes that possible.
—
Reply to this email directly, view it on GitHub
<#47 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFMPQZSP2HGDITJIHO2VOT2THRWTAVCNFSM6AAAAABYPORJC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBXGE2TSNJUGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Indeed it does. See the source for the many possibilities. I did not see any compelling reason to limit this to only |
|
It is not clear we need many possibilities here. We only create one motor
and one detector for testing.
Is this something that anyone will use or even test in the future?
…On Fri, Mar 7, 2025 at 12:47 PM Pete R Jemian ***@***.***> wrote:
factory implies the creation of many.
Indeed it does. See the source
<https://github.com/bluesky/ophyd/blob/main/ophyd/sim.py> for the many
possibilities.
—
Reply to this email directly, view it on GitHub
<#47 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFMPQYADGXSINW3T2KGVU32THST3AVCNFSM6AAAAABYPORJC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBXGE3TGMZWGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: prjemian]*prjemian* left a comment (BCDA-APS/BITS#47)
<#47 (comment)>
factory implies the creation of many.
Indeed it does. See the source
<https://github.com/bluesky/ophyd/blob/main/ophyd/sim.py> for the many
possibilities.
—
Reply to this email directly, view it on GitHub
<#47 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFMPQYADGXSINW3T2KGVU32THST3AVCNFSM6AAAAABYPORJC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBXGE3TGMZWGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
…S/BITS into switch_case_organization
Pull Request Test Coverage Report for Build 13797639520Details
💛 - Coveralls |
prjemian
reviewed
Mar 8, 2025
prjemian
reviewed
Mar 8, 2025
|
Yes. I think so.
…On Sat, Mar 8, 2025 at 10:32 AM Pete R Jemian ***@***.***> wrote:
***@***.**** commented on this pull request.
Still in draft?
—
Reply to this email directly, view it on GitHub
<#47 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEFMPQ5N5HGL2XIAMVSYAPD2TMLSHAVCNFSM6AAAAABYPORJC2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNRZGIZDGNBZGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Probably useful for our isolation efforts to discuss |
|
Still not passing tests. |
…ting if a variable is imported of not based on iconfig
MDecarabas
changed the title
MDecarabas
commented
Mar 11, 2025
MDecarabas
commented
Mar 11, 2025
Comment on lines
+63
to
+65
| version: str = iconfig.get( | ||
| "ICONFIG_VERSION", "0.0.0" | ||
| ) # TODO: Will anyone ever have a wrong catalog version? |
There was a problem hiding this comment.
What are the odds the iconfig version is incorrect?
There was a problem hiding this comment.
The idea of versioning this iconfig.yml is to define what keys are expected. Bump the version when the structure changes. This template will spawn living repositories, each with their own evolution.
There was a problem hiding this comment.
Doesn't this test that:
- version exists
- meets or exceeds some minimum
This line in iconfig is never activated. Bin it? |
ravescovi
approved these changes
Mar 11, 2025
|
@MDecarabas - You commented on: BITS/src/instrument/configs/iconfig.yml Line 21 in 3b00fcc Should keep as comment, but edit the comment which is invalid YAML synatx: # SCAN_ID_PV: "IOC:scan_id_pv"The beamline that has a PV for this would uncomment this line and edit the PV value. For them, their |
prjemian
reviewed
Mar 11, 2025
prjemian
reviewed
Mar 11, 2025
Co-authored-by: Pete R Jemian <[email protected]>
prjemian
reviewed
Mar 11, 2025
src/instrument/configs/iconfig.yml
Outdated
| @@ -63,8 +67,8 @@ APS_DEVICES_FILE: devices_aps_only.yml | |||
| OPHYD: | |||
| ### Control layer for ophyd to communicate with EPICS. | |||
| ### Default: PyEpics | |||
| ### Choices: "PyEpics" or "caproto" | |||
| # CONTROL_LAYER: caproto | |||
| ### Choices: "PyEpics" or "caproto" # caproto is not yet supported | |||
There was a problem hiding this comment.
Why? We need to keep the ability to make a field replacement. Two times in the past five years at APS.
There was a problem hiding this comment.
Perhaps this might say
Suggested change
| ### Choices: "PyEpics" or "caproto" # caproto is not yet supported | |
| ### Choices: "PyEpics" or "caproto" # caproto is not yet tested with this package |
prjemian
reviewed
Mar 11, 2025
| @@ -50,6 +50,7 @@ dependencies = [ | |||
| "pyRestTable", | |||
| "pysumreg", | |||
| "qtpy", | |||
| "caproto", | |||
prjemian
reviewed
Mar 11, 2025
| @@ -76,6 +76,7 @@ def test_StoredDict(md_file): | |||
| # Add another key. | |||
| sdict["bee"] = "bumble" | |||
| sdict.flush() | |||
| print(f"\n\nthis is the md_file: {md_file}\n\n") | |||
There was a problem hiding this comment.
pytest usually swallows stdout. Can this be removed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Addressing Switch cases. PR is also gonna be used to test Issues: #43
make_devices()#44Type of change