-
Notifications
You must be signed in to change notification settings - Fork 0
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
Tickets/DM-40219 #144
Tickets/DM-40219 #144
Conversation
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.
Looking good. Here an initial round of comments. Also, please, rebase to develop.
self.mtcs = None | ||
|
||
# Set the add_remotes flag | ||
self.add_remotes = add_remotes |
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.
See my comment in #137 about add_remote
. You can apply the same strategy here.
self.add_remotes = add_remotes | ||
|
||
# Create the DOF vector | ||
self.dofs = np.zeros(50) |
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.
Maybe use
self.dofs = np.zeros(len(DOFName))
?
await self.assert_feasibility() | ||
|
||
await self.checkpoint("Applying DOF offset...") | ||
await self.mtcs.mtaos.cmd_offsetDOF(self.dofs) |
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.
This is not going to work. To execute the command you need to call set_start
, however with arrays like this you will need to do something slightly different. something like this should work:
offset_dof_data = self.mtcs.mtaos.cmd_offsetDOF.DataType()
for i, dof_offset in enumerate(self.dofs):
offset_dof_data.value[i] = dof_offset
await self.mtcs.mtaos.cmd_offsetDOF.start(data=offset_dof_data)
tests/test_maintel_apply_dof.py
Outdated
standardscripts.BaseScriptTestCase, unittest.IsolatedAsyncioTestCase | ||
): | ||
async def basic_make_script(self, index): | ||
self.script = ApplyDOF(index=index, add_remotes=False) |
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.
See my comment about add_remotes
in the other PR #137
tests/test_maintel_apply_dof.py
Outdated
async with self.make_script(): | ||
config_dofs = {"M2_dz": 0.2, "Cam_dy": 0.3, "M1M3_B1": 0.5, "M2_B14": 0.7} | ||
|
||
await self.configure_script( |
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.
You can do
await self.configure_script(**config_dofs)
instead.
tests/test_maintel_apply_dof.py
Outdated
M2_B14=config_dofs["M2_B14"], | ||
) | ||
|
||
dofs = np.zeros(50) |
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.
use dofs = np.zeros(len(DOFName))
tests/test_maintel_apply_dof.py
Outdated
async with self.make_script(): | ||
config_dofs = {"M2_dz": 0.2, "Cam_dy": 0.3, "M1M3_B1": 0.5, "M2_B14": 0.7} | ||
|
||
await self.configure_script( |
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.
You can do
await self.configure_script(**config_dofs)
instead.
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.
Some additional comments. Please remember to rebase.
tests/test_maintel_apply_dof.py
Outdated
|
||
random.seed(47) # for set_random_lsst_dds_partition_prefix | ||
|
||
logging.basicConfig() |
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.
you can remove this
await self.assert_feasibility() | ||
|
||
await self.checkpoint("Applying DOF offset...") | ||
offset_dof_data = self.mtcs.mtaos.cmd_offsetDOF.DataType() |
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.
This should be self.mtcs.rem.mtaos.cmd_offsetDOF.DataType()
, e.g., it is missing the rem
between mtcs
and mtaos
.
offset_dof_data = self.mtcs.mtaos.cmd_offsetDOF.DataType() | ||
for i, dof_offset in enumerate(self.dofs): | ||
offset_dof_data.value[i] = dof_offset | ||
await self.mtcs.mtaos.cmd_offsetDOF.start(data=offset_dof_data) |
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.
Same as above, it is missing the rem
between mtcs
and mtaos
, e.g.;
await self.mtcs.rem.mtaos.cmd_offsetDOF.start(data=offset_dof_data)
tests/test_maintel_apply_dof.py
Outdated
intended_usage=MTCSUsages.DryTest, | ||
log=self.script.log, | ||
) | ||
self.mtcs.mtaos.cmd_offsetDOF.DataType = unittest.mock.PropertyMock( |
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.
This should probably be self.script.mtcs
. However, I am not sure this is needed or if this is actually what you want. Because in the script you call:
offset_dof_data = self.mtcs.mtaos.cmd_offsetDOF.DataType()
which is a method not a property.
I think what you want to do is something like:
self.script.mtcs.rem.mtaos = unittest.mock.AsyncMock()
self.script.mtcs.rem.mtaos.attach_mock(
unittest.mock.Mock(
return_value=types.SimpleNamespace(value=np.zeros(50))
),
"DataType",
)
self.script.mtcs.assert_all_enabled = unittest.mock.AsyncMock()
I don't think you need anything else in terms of mocking.
|
||
# Run the script | ||
await self.run_script() | ||
|
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.
Maybe add assertions about the things that you call and await that are mocks? e.g.,
self.script.mtcs.mtaos.cmd_offsetDOF.DataType.assert_called()
self.script.mtcs.rem.mtaos.cmd_offsetDOF.start.assert_awaited_once()
973fad8
to
93065c5
Compare
93065c5
to
93d9150
Compare
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
No description provided.