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

Dev small checks in merging #171

Merged
merged 7 commits into from
Jul 19, 2023
Merged

Dev small checks in merging #171

merged 7 commits into from
Jul 19, 2023

Conversation

bdestombe
Copy link
Collaborator

@bdestombe bdestombe commented Jul 17, 2023

The number of measurements of the forward and backward channels might get out of sync if the device shuts down before the measurement of the last channel is complete. A function is added skips all measurements that are not accompanied with a partner channel.

If all measurements are recorded: fw_t0, bw_t0, fw_t1, bw_t1, fw_t2, bw_t2, .. > all are passed

If some are missing the accompanying measurement is skipped:

  • fw_t0, bw_t0, bw_t1, fw_t2, bw_t2, .. > fw_t0, bw_t0, fw_t2, bw_t2, ..
  • fw_t0, bw_t0, fw_t1, fw_t2, bw_t2, .. > fw_t0, bw_t0, fw_t2, bw_t2, ..
  • fw_t0, bw_t0, bw_t1, fw_t2, fw_t3, bw_t3, .. > fw_t0, bw_t0, fw_t3, bw_t3, ..

Not perfect and the following situation is not caught:

  • fw_t0, bw_t0, fw_t1, bw_t2, fw_t3, bw_t3, .. > fw_t0, bw_t0, fw_t1, bw_t2, fw_t3, bw_t3, ..

Mixing forward and backward channels can be problematic when there is a pause after measuring all channels. This routine checks that the lowest channel number is measured first (aka the forward channel), but it doesn't catch the last case as it doens't know that fw_t1 and bw_t2 belong to different cycles.

@bdestombe bdestombe self-assigned this Jul 17, 2023
Copy link
Collaborator

@BSchilperoort BSchilperoort left a comment

Choose a reason for hiding this comment

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

This is for when a user has set up a manual double ended mode right? Instead of relying on the machine creating, e.g., a single xml file which contains both the forward and backward measurements.

It would also be good to update the changelog!

Skips measurements for which there is no partner measurement. Little
protection against swapping fw and bw is implemented.

If all measurements are recorded: fw_t0, bw_t0, fw_t1, bw_t1, fw_t2, bw_t2, ..
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test for this?

You can then also add an xfail for the uncaught situation :)

Pytest's parameterize might come in useful for this to keep the test short.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea! Just implemented it.

([1, 2], [], [1, 2]),
([], [1, 2], [1, 2]),
([1], [2], [1, 2]),
pytest.param([2], [1], [1, 2], marks=pytest.mark.xfail)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :D

@bdestombe bdestombe merged commit bc507f1 into main Jul 19, 2023
@bdestombe bdestombe deleted the dev_small_checks_in-merging branch July 19, 2023 08:18
@BSchilperoort
Copy link
Collaborator

Hi! You didn't update the changelog ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants