Skip to content
This repository was archived by the owner on Sep 2, 2024. It is now read-only.

734 grid using callbacks#901

Merged
5u6in merged 16 commits into
mainfrom
734_grid_using_callbacks
Sep 20, 2023
Merged

734 grid using callbacks#901
5u6in merged 16 commits into
mainfrom
734_grid_using_callbacks

Conversation

@5u6in

@5u6in 5u6in commented Sep 20, 2023

Copy link
Copy Markdown
Contributor

Fixes #734

Tests using:

def _fake_grid_detection(
    parameters: OAVParameters,
    snapshot_template: str,
    snapshot_dir: str,
    grid_width_microns: float = 0,
    box_size_um: float = 0.0

have been skipped by using @pytest.mark.skip as _fake_grid_detection does not have real enough parameters to pass the tests. I will raise a new issue for this.

To test:

  1. Confirm (relevant) tests passes

@5u6in
5u6in requested review from DominicOram and d-perl September 20, 2023 10:04

@d-perl d-perl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. Because we have discussed this so much in person I don't think there's any more to do for this change.

assert gridscan_params.x_start == 0.0005
assert gridscan_params.y1_start == -0.0001
assert gridscan_params.z1_start == -0.0001
assert gridscan_params.x_start == 0.1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did these values change?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yh, it's worrying that this changed, why did it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

gridscan_params = GridScanParams() is located in fast_grid_scan.py.
The values for x_start, y1_start and z1_start are defined in the GridScanParams class to begin with.
I change it to 0.1 since that is what the test is expecting in order to pass.
Is it wise to change the values of 0.0005, -0.0001, -0.0001 for x_start, y1_start and z1_start respectively in GridScanParams()?

@d-perl d-perl Sep 22, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, that won't fix the problem.
Previously, the default parameter of x_start = 0.1 in GridScanParams was modified in grid_detection_plan in line 167: out_parameters.x_start = start_positions[0][0], so the default value was replaced before the assertion in the test is run.

What is different about the way start_positions[0][0] used to be calculated in grid_detection_plan compared to how GridDetectionCallback calculates start_positions now?

@5u6in
5u6in merged commit c10c772 into main Sep 20, 2023
@5u6in
5u6in deleted the 734_grid_using_callbacks branch September 20, 2023 12:21

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great, some comments in there

Comment on lines +9 to +10
self.box_widths: list = []
self.no_of_boxes: list = []

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should: Are these used? Can we remove them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just did a quick test and debug. They are not used so it will be removed

assert gridscan_params.x_start == 0.0005
assert gridscan_params.y1_start == -0.0001
assert gridscan_params.z1_start == -0.0001
assert gridscan_params.x_start == 0.1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yh, it's worrying that this changed, why did it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get grid parameters out of grid detection using callbacks

3 participants