Allow relative z in multi_d_acquisition_events

[ianhi]

Closes: #153

This should maintain all the previous behaviors but also allow for specifying z positions in these new ways:

• z_positions as 2D array
• z_positions as 1D array
• z_positions as 1D array + relative positions

which is hopefully made clear in the new docs page I added for this.

Stuff like this is a great candidate for pytest, I thought about adding that but didn't becuase you already have a large user automated testing DIR that I wasn't sure would work with pytest on CI. Let me know what would be best

[henrypinkard]

A couple things:

• The contents and the description of the notebook are not in the same order on the applications page
• In the notebook I think you can delete the part with np.c_ and just show hstack, since this is showing a feature of numpy, not pycromanager
• I was a bit confused about the docstring for multi_d_acquisition_events, specifically the "for each xy point, or 2D (shape: (N, n_z) ) to fully specify the xyz points". After reading the notebook I get what n_z is supposed to be. But I think it might be a simpler interface for this to have xyz_positions and xy_positions, where the former automatically puts you into relative z mode and any z stacks are relative to it, while the latter defaults to absolute z mode if a z stack is provided. This wway i think would fit in more with the existing API, and it would leave less work for the user in the case of different z stacks at different positions (i.e. just supply xyz_positions and z_start, z_end, z_step without having to construct the 2d array yourself). What do you think?
• The additional descriptions of channels/time in the notebook is really nice. It could be good to link to this notebook from the features page on MDAs

[henrypinkard]

Also, yes, it would be great to some more stuff built into automatic testing. That test directory is not automated at the moment, I just copy and paste them to quickly debug when I think things are broken. @bryantChhun has also been thinking about adding automated testing. Maybe better to continue this conversation on the issue for it

[ianhi]

The contents and the description of the notebook are not in the same order on the applications page

The additional descriptions of channels/time in the notebook is really nice. It could be good to link to this notebook from the features page on MDAs

I'm not sure I understand what the order is meant to be. Is it chronological? Also, if anything this notebook is more of a tutorial than an application, would it make sense to fold it into the page on MDAs?

I was a bit confused about the docstring for multi_d_acquisition_events, specifically the "for each xy point, or 2D (shape: (N, n_z) ) to fully specify the xyz points". After reading the notebook I get what n_z is supposed to be. But I think it might be a simpler interface for this to have xyz_positions and xy_positions, where the former automatically puts you into relative z mode and any z stacks are relative to it, while the latter defaults to absolute z mode if a z stack is provided. This wway i think would fit in more with the existing API, and it would leave less work for the user in the case of different z stacks at different positions (i.e. just supply xyz_positions and z_start, z_end, z_step without having to construct the 2d array yourself). What do you think?

I was probably doing some premature optimization here, the current setup allows for different non-uniform stepped z stacks for each positions. So you could have
pos1: z = [-3,-1,0]
pos2: z= [-10, -1, 0]

but that's probably not necessary to include.

To be explicit you're proposing the following call signatures:

# Gives relative z
multi_d_acquisition_events(xyz_positions = xyz)
multi_d_acquisition_events(xyz_positions = xyz, z_start=-1, z_stop = 1, z_step=1)

# absolute z
multi_d_acquisition_events(z_start=0, z_stop=10,z_step=1)
multi_d_acquisition_events(xy_positions = xy, z_start=0, z_stop=10,z_step=1)

I'd suggest adding the option of an array of relative z values:
(this should give the same result as xyz_positions passed with z_start et al)

multi_d_acquisition_events(xyz_positions = xyz, z_rel = np.linspace(-1,1,3))

The only downsides I see are:

1. you need to know current xy point in order to make a single position z stack
2. not totally clear to me how best to handle an order string like zp
   • This PR already probably won't work in that case though :/

[henrypinkard]

The additional descriptions of channels/time in the notebook is really nice. It could be good to link to this notebook from the features page on MDAs

I'm not sure I understand what the order is meant to be. Is it chronological? Also, if anything this notebook is more of a tutorial than an application, would it make sense to fold it into the page on MDAs?

Vaguely scales with complexity. Either position in the ordering is fine, you've just got it first in the TOC but second on the actual list right now (or vice versa). I think given the density of it, it makes more sense to have it with applications.

I was a bit confused about the docstring for multi_d_acquisition_events, specifically the "for each xy point, or 2D (shape: (N, n_z) ) to fully specify the xyz points". After reading the notebook I get what n_z is supposed to be. But I think it might be a simpler interface for this to have xyz_positions and xy_positions, where the former automatically puts you into relative z mode and any z stacks are relative to it, while the latter defaults to absolute z mode if a z stack is provided. This wway i think would fit in more with the existing API, and it would leave less work for the user in the case of different z stacks at different positions (i.e. just supply xyz_positions and z_start, z_end, z_step without having to construct the 2d array yourself). What do you think?

I was probably doing some premature optimization here, the current setup allows for different non-uniform stepped z stacks for each positions. So you could have
pos1: z = [-3,-1,0]
pos2: z= [-10, -1, 0]

but that's probably not necessary to include.

Makes sense. Generally speaking I don't think its worth the effort to try to get every edge case into this function, since for customized stuff you should probably just be going a level deeper and creating events manually anyway. In my mind z-stacks with custom nonuniform step spacing would fall under this

To be explicit you're proposing the following call signatures:

# Gives relative z
multi_d_acquisition_events(xyz_positions = xyz)
multi_d_acquisition_events(xyz_positions = xyz, z_start=-1, z_stop = 1, z_step=1)

# absolute z
multi_d_acquisition_events(z_start=0, z_stop=10,z_step=1)
multi_d_acquisition_events(xy_positions = xy, z_start=0, z_stop=10,z_step=1)

Yeah exactly. Or maybe a relative_z=True argument could be added to make this explicit. What do you prefer?

I'd suggest adding the option of an array of relative z values:
(this should give the same result as xyz_positions passed with z_start et al)

multi_d_acquisition_events(xyz_positions = xyz, z_rel = np.linspace(-1,1,3))

I think its better to make an example of doing this manually than to have an explicit option. The purpose of this function is a a convenience for people who dont want to dig any deeper into the API, so I think the fewer ways of doing things, the better

The only downsides I see are:

1. you need to know current xy point in order to make a single position z stack

Maybe this is an argument for relative_z=True argument

2. not totally clear to me how best to handle an order string like `zp`

Fine to just throw in an exception in the cases that it doesnt make sense

[ianhi]

@henrypinkard I think this is good now. I ended up adding github actions to run the pytest tests I added for MDA, but I think you need to change a setting to get them to run on PRs. I opened a PR against my fork and the tests all ran there: ianhi#1 I think you need to modify these settings:

[ianhi]

Yeah exactly. Or maybe a relative_z=True argument could be added to make this explicit. What do you prefer?

I went for raising errors rather than adding yet more kwargs.

Makes sense. Generally speaking I don't think its worth the effort to try to get every edge case into this function,

sounds good! It indeed simplified it greatly.

@bryantChhun has also been thinking about adding automated testing. Maybe better to continue this conversation on the issue for it

In a moment of forgiveness is better than permission I went ahead and greated github actions + pytest tests for the content of this PR. Testing the actual functionality of the integration with μmanager would be much more complex though so I'll leave that to the other issue.

pytest supports multiple test directories and I think its pretty standard to have one inside the pycromanager folder so I added one there for testing things that are contained to python, should be easy enough to add in the other test directory or combine them in future. But I wanted to keep the automated and non-automated stuff separate for now.

[ianhi]

This includes one more point than the old version. To me this way make more sense but it is clearly backwards incompatible.. let me know if you'd prefer this to be

Suggested change

	z_rel = np.arange(z_start, z_end + z_step, z_step)
	z_rel = np.arange(z_start, z_end, z_step)

[henrypinkard]

Yeah I think that's fine and probably an upgrade

[bryantChhun]

In a moment of forgiveness is better than permission I went ahead and greated github actions + pytest tests for the content of this PR. Testing the actual functionality of the integration with μmanager would be much more complex though so I'll leave that to the other issue.

no worries! I didn't do much other than add some mocks and a couple tests. Will create a PR soon. And in general, I never discourage anyone from writing tests for their code :-)

There was a discussion by email about the more complex testing with umanager (apologies, we agreed we'll move those discussions to github issues). The general conclusion was that other umanager work (release of 2.0) is taking priority. Continuous Integration with micromanager will require solving some issues in the 2.1 release..

[henrypinkard]

I think this is good now. I ended up adding github actions to run the pytest tests I added for MDA, but I think you need to change a setting to get them to run on PRs. I opened a PR against my fork and the tests all ran there: ianhi#1 I think you need to modify these settings:

I looked on the settings but it was the same as the picture you posted -- allow all actions was already selected.

[henrypinkard]

Any other ideas on how to set it up like you had in that link?

[ianhi]

Any other ideas on how to set it up like you had in that link?

🤔 hmm. I would have thought that was all that was necessary. I suppose there isn't an error in the yaml file as they ran on my fork. It looks as though they did run once you merged it:

I'm going to try opening a dummy PR to see if they will trigger on that now that they're properly in master, maybe its different than my repo bc I don't own it.

[ianhi]

See #162. looks as though the tests work on newly created PR