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

Adds hook that runs after all hardware except for the Z drive has been moved in postion. #118

Merged
merged 6 commits into from
Apr 13, 2024

Conversation

nicost
Copy link
Member

@nicost nicost commented Apr 11, 2024

This hook enables autofocussing. Also fixes an issue where events where not sequences if a 1D drive had been added to stage positions. Improved some error reporting, and bumped version to 0.37.0.

@nicost nicost requested a review from henrypinkard April 11, 2024 23:37
@henrypinkard
Copy link
Member

2 things:

  1. You can changed the numbers of the hooks if you want as described in other issue
  2. If you're adding a new notification, you need to add the same to the python side so that it knows how to interpret it. I'm hoping to relatively soon write tests for the notification system so that these things behave as expected and its easy to detect errors

@nicost
Copy link
Member Author

nicost commented Apr 12, 2024

  1. Done.
  2. I tried setting up the pycromanager development environmnet, but quickly got lost in Python dependency problems and IDE setup issues. Do you mind adding these few lines, as I do not have any need for pycromanager other than adding these tests?

@nicost
Copy link
Member Author

nicost commented Apr 13, 2024

All right, I am completely confused by the Python side. There is a complete copy there of AcqEngJ that needs to be modified the same way? I opened a PR, but I have no way of testing (or even compiling and running that code).

@nicost nicost merged commit 198aabe into micro-manager:main Apr 13, 2024
@nicost nicost deleted the errorReporing branch April 13, 2024 17:25
@henrypinkard
Copy link
Member

henrypinkard commented Apr 15, 2024

Thanks! To clarify, I only meant that one line adding the type of notification, not full functionality in AcqEngPy.

A little more explanation: Both micro-manager and pycro-manager depend on AcqEngJ. Thus, we don't want changes to AcqEngJ merging without pycro-manager testing them. For this reason, updates to AcqEngJ's version automatically propagate to pycro-manager through Github actions, where they are tested, and then to micro-manager, where they are tested once again.

You should not, in the future, circumvent this system and manually update the AcqEngJ version in ivy.xml as you did in micro-manager/micro-manager#1922, because this has the potential to break pycro-manager, which has happened this time (micro-manager/pycro-manager#760). This would have been avoided had the updates been automatic, since the automatic testing system caught the error, thus preventing the version update of AcqEngJ from automatically propagating through pycro-manager into MMStudio (micro-manager/pycro-manager#759).

2. If you're adding a new notification, you need to add the same to the python side so that it knows how to interpret it.

What I was suggesting here was a change pre-emptively fix the problem that would arise, which is that pycro-manager doesn't know how to interpret the new notification type. I believe this is just one or two lines of code, not a full suite of changes to bring AcqEngPy up to parity.

I will look into fix for this

@nicost
Copy link
Member Author

nicost commented Apr 15, 2024

Sorry for creating chaos! I literally tried to follow what you told me to do including: "I think you could manually change the version number needed of AcqEngJ in ivy". I still do not understand the complete build system, with multiple dependencies, so your help getting this right is highly appreciated.

@henrypinkard
Copy link
Member

No problem, its a complicated system that's hard to describe in quick messages. We can go over it at the next developer meeting. For now, good rule of thumb is: let the ivy file get auto updated with acq eng versions

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

Successfully merging this pull request may close these issues.

2 participants