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

Acquisition Rate of planar mode is not used #145

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Acquisition Rate of planar mode is not used #145

merged 1 commit into from
Feb 28, 2024

Conversation

fedem-p
Copy link
Member

@fedem-p fedem-p commented Jul 27, 2022

Here's a simple fix to the second problem listed in #144.

The Issue

changing the parameter regarding the how many planes per second should be recorded doesn't update the saving parameter values

The top bar gui receive the info to display (number of volumes) from the save_status inside state.py
The save_status is changed in the stacksaver using the received save_params
the save_params is a property of the state which converts the appropriate settings in saving parameters

flowchart LR;
    stacksaver -->save_status;
    save_params -->stacksaver;
    save_status -->top_bar_gui;

    subgraph state
settings --> save_params;
    save_status;
    save_params;
    end 

    subgraph top_guy
    top_bar_gui;
    end

    subgraph stream_saver
    stacksaver;
    end
Loading

Now, the problem relies in the fact that save_params and the function convert_save_params only accepted/used ZRecordingSettings to extract the scanning_settings.
This works well for volumetric mode but fails to account for any changes in the single_plane_settings.

The Solution

I changed three functions:

  1. save_params to determine whether it's planar or volumetric mode and pass to convert_save_params the appropriate settings (the default is volumetric)
  2. convert_save_params to accept and correctly extract information from both volume_settings and single_plane_settings (in particular -> n_planes is set to 0 if in planar mode)
  3. get_voxel_size to compute a voxel size even in planar_mode -> z voxel is left at 1

What's left

I'd like to hear some feedback regarding two main values:

  • n_planes which is set to 0 in planar_mode -> maybe it could be better to set it to None?
  • the default voxel size for the z axis in planar mode is set to 1 -> or maybe it should only return a tuple of 2 values if it's planar mode?
  • unfortunately this doesn't solve the first problem listed in Timing and other related bugs #144 which would now affect also the planar mode (probably better addressed in another PR)
  • write docstrings

@fedem-p fedem-p added the bug Something isn't working label Jul 27, 2022
@fedem-p fedem-p self-assigned this Jul 27, 2022
@fedem-p fedem-p requested review from vigji, diegoasua and vilim July 27, 2022 13:09
Comment on lines +234 to +241
if isinstance(scanning_settings, SinglePlaneSettings):
inter_plane = 1
else:
scan_length = (
scanning_settings.piezo_scan_range[1]
- scanning_settings.piezo_scan_range[0]
)
inter_plane = scan_length / scanning_settings.n_planes
Copy link
Member Author

Choose a reason for hiding this comment

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

Inside get_voxel_size:
checks if the scanning_settings are single_plane_settings and if so sets the z_voxel size to 1

Comment on lines -251 to +261
n_planes = scanning_settings.n_planes - (
scanning_settings.n_skip_start + scanning_settings.n_skip_end
)
if isinstance(scanning_settings, SinglePlaneSettings):
n_planes = 0
else:
n_planes = scanning_settings.n_planes - (
scanning_settings.n_skip_start + scanning_settings.n_skip_end
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Inside convert_save_params checks if the settings are for planar mode, and if so, sets n_planes to 0

Comment on lines +483 to +497
if self.global_state == GlobalState.PLANAR_PREVIEW:
save_p = convert_save_params(
self.save_settings,
self.single_plane_settings,
self.camera_settings,
self.trigger_settings,
)
else:
save_p = convert_save_params(
self.save_settings,
self.volume_setting,
self.camera_settings,
self.trigger_settings,
)
return save_p
Copy link
Member Author

Choose a reason for hiding this comment

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

inside save_params checks if we are in planar mode and returns the planar settings - if not defaults to volume settings

@coveralls
Copy link

coveralls commented Jul 27, 2022

Pull Request Test Coverage Report for Build 2746865186

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 9 of 12 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 79.765%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sashimi/state.py 9 12 75.0%
Totals Coverage Status
Change from base Build 2744873404: 0.02%
Covered Lines: 2373
Relevant Lines: 2975

💛 - Coveralls

@diegoasua
Copy link
Member

Let's keep n_planes as 0 so that there is no casting. And it is actually more informative. On the other hand Int to None is not exactly a casting operation but it could have some undesirable effects downstream. In particular, I am not 100% sure that the external_communication process will handle this correctly. It should. And testing should catch that otherwise. But certainty is not an exact science in Sashimi as you know. And overall it makes more sense to specify that there were 0 planes than None planes, as long as this is documented as being a single-plane recording. Same goes for voxel size, just setting to 1 is fine.

TLDR keep n_planes as 0

@diegoasua
Copy link
Member

By the way that diagram is super cool. Which tool did you use?

@vigji
Copy link
Member

vigji commented Jul 28, 2022

I would say that the defaults you use would depend on the saving format.

  • If the output in planar mode is a (t, 1, w, h) stack, then n_planes should be 1 and voxel_size=(None, w_res, h_res)
  • If the output in planar mode is a (t, w, h) stack, then n_planes should be None/0 and voxel_size=(w_res, h_res)

@fedem-p
Copy link
Member Author

fedem-p commented Jul 28, 2022

By the way that diagram is super cool. Which tool did you use?

Thanks 👍 😄
I used a github codeblock with mermaid (I literally found it yesterday and is basically a languagge for writing flowcharts and other cool things) :)

@nvbln nvbln marked this pull request as ready for review February 28, 2024 11:54
@nvbln
Copy link
Contributor

nvbln commented Feb 28, 2024

Since there have been no comments in the last two years and the PR seems to work I'll merge it.

@nvbln nvbln merged commit ee1fe72 into master Feb 28, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants