Updates to support rogue 6 - #485
tristpinsm wants to merge 22 commits into
Conversation
64e7d8d to
1a806e0
Compare
936ed42 to
b24c50d
Compare
|
This should be ready for review and merge now |
77b88b3 to
45d974c
Compare
BrianJKoopman
left a comment
BrianJKoopman
left a comment
There was a problem hiding this comment.
This looks good to me, with the caveat that I'm not really familiar enough with the epics/zmq details and inner pysmurf stuff to really comment on the changes in the operations. My comments focus on the packaging, and I'm wondering about the epics dependency.
There was a problem hiding this comment.
Commenting here for lack of a better place -- does this PR totally eliminate the epics dependency?
If so, there are a couple of lingering spots it's mentioned/used:
$ git grep -e 'epics'
docs/det_config.rst:DetConfig object will automaticlly determine the epics root and set the
scripts/resonator_fitting.py: parser.add_argument('--epics-root', default='smurf_server_s2')
scripts/resonator_fitting.py: epics_root = args.epics_root,
scripts/restart_rssi.py:import epics
scripts/restart_rssi.py:ipbyeng=[epics.PV(name).get(as_string=True) for name in [client_remote_ip0.format(iudpe) for iudpe in range(nudpe)]]
scripts/restart_rssi.py: validcntpv=epics.PV(rssi_validcnt.format(slots[slot]['engine']))
scripts/restart_rssi.py: restartconnpv=epics.PV(rssi_restartconn.format(slots[slot]['engine']))
scripts/set_stream.py: parser.add_argument('--epics-root', type=str, default=None)
scripts/set_stream.py: dump_configs=args.dump_configs, epics_root=args.epics_root,
sodetlib/util.py: existing pysmurf function so we can set both PV's in a single epics call,
tests/input/pysmurf.cfg:"epics_root" : "test_epics",
tests/test_jackhammer.py: f"connection to {epics_server}"
I think at least tests/test_jackhammer.py, the description in sodetlib/util.py, and the docs in docs/det_config.rst should probably be addressed. The scripts/ directory I'm less sure what to do about. If dropping epics in those scripts is straight forward, let's do that too.
tests/input/pysmurf.cfg and generally the tests/test_det_config.py file looks like it needs attention -- it's not passing for me on a fresh install from master or from this branch. That doesn't have to happen in this PR though.
There was a problem hiding this comment.
that's a good point -- the scripts directory slipped by me. Shouldn't be too hard to adapt those. Are those tests configured to run? I haven't seen them in CI.
Thanks for taking a look!
There was a problem hiding this comment.
Great, I think there are some cases where the scripts/ are still used -- though I'd be happy to be wrong about that. (They're sort of awkward here, since they're not really a part of the package that gets published to PyPI, but last I looked I think jackhammer interacted with that directory in some way.)
Also, no, there's no CI to run those tests (which is probably why the breakage went without anyone noticing). If you want to set that up (either here or in a new PR), I'd be happy to review that too. There aren't many tests in this repo, so I think motivation was low.
For a reason I don't understand, setting a default of
opt_args={} was being ignored and populated with parameter
values.
Two timeouts: - An overall timeout for checking on the server - A timeout for individual ping attempts. Also enforce this wait between attempts.
S.get_eta_phase_array returns radians, but the 'eta_phase' key of a resonance dict is degrees. run_grad_descent_and_eta_scan wrote the radians value into res['eta_phase'] with update_tune=True, so the resulting tune file held radians under a degrees key and every phase came back ~57x too small when reloaded. plot_channel_resonance made the opposite mistake, converting the already-radians array with np.deg2rad before rotating the response, so the circle plot was drawn at the wrong angle. In get_full_band_sweep, restore the eta magnitude before the phase. Eta is stored in firmware as Cartesian etaI/etaQ and each setter recomputes from the other's current value, so setting the phase while the magnitude is zero writes (0, 0) and drops the phase. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rs." This reverts commit d3f692a.
N.B. I suspect these scripts may be broken for other reasons and I haven't attempted to fix them.
Deprecated in favour of the RestartRssi command added to pysmurf in the meantime.
ff41525 to
0979e7c
Compare
This branch has not been deployed
2 participants
Tested on SLAC system