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

Add image tags to events #90

Closed
nicost opened this issue Jun 3, 2023 · 11 comments · Fixed by #91
Closed

Add image tags to events #90

nicost opened this issue Jun 3, 2023 · 11 comments · Fixed by #91

Comments

@nicost
Copy link
Member

nicost commented Jun 3, 2023

Currently, it is difficult to attach information to an event to be passed onto a tagged image. For instance, I am currently faking an axis so that the DataSink can see which event was responsible for a given image. Another example is including the name of a position (which possibly can be done by passing the position list to the DataSink). In general, it seems useful for an event to be able to include arbitrary information to the image it will generate.

The most straight forward way seems to include a field "tags", possibly of type Map<String, String> to the event, with getters and setters. The Acquisition Engine then would add those to a TaggedImage as key-value pairs.

Happy to take a stab at this.

@henrypinkard
Copy link
Member

This automatically propagated the new version to update pycro-manager, and the test failed (the old one--I haven't added the new one yet) because:

pycromanager\test\test_hook_functions.py:65: AssertionError
=========================== short test summary info ===========================
FAILED pycromanager/test/test_hook_functions.py::test_event_serialize_and_deserialize - AssertionError: assert {'axes': {'ch...], 'tags': {}} == {'axes': {'ch...nel', 'DAPI']}
  Omitting 2 identical items, use -vv to show
  Left contains 1 more item:
  {'tags': {}}
  Full diff:
  - {'axes': {'channel': 'DAPI'}, 'config_group': ['Channel', 'DAPI']}
  + {'axes': {'channel': 'DAPI'}, 'config_group': ['Channel', 'DAPI'], 'tags': {}}
  ?       

Im guessing this is an error in the toJSON function. It adds an empty JSONobject for tags, when the expected behavior is that this does not get added if no tags are present.

@henrypinkard henrypinkard reopened this Jun 7, 2023
@nicost
Copy link
Member Author

nicost commented Jun 7, 2023

Fair enough. Just pushed a fix, not sure why it does not show here.

if (e.getTags() != null && !e.getTags().isEmpty()) {

@nicost
Copy link
Member Author

nicost commented Jun 7, 2023

PR #92 contains this putative fix.

I am unable to run these tests locally. When running the snippet you posted above, I get:

Traceback (most recent call last):
  File "C:\projects\personal\pycro-manager\pycromanager\test\tmp.py", line 24, in <module>
    with Acquisition(show_display=False, pre_hardware_hook_fn=hook_fn) as acq:
NameError: name 'Acquisition' is not defined

When running one of the test scripts directly, them output is empty. When running them in PyCharm, I get:

ImportError while loading conftest 'C:\projects\personal\pycro-manager\pycromanager\test\conftest.py'.
conftest.py:6: in <module>
    import wget
E   ModuleNotFoundError: No module named 'wget'

Relying on the server to run them after a merge is not really a pleasant workflow (especially considering the 10 minutes work around).

@henrypinkard
Copy link
Member

Traceback (most recent call last):
File "C:\projects\personal\pycro-manager\pycromanager\test\tmp.py", line 24, in
with Acquisition(show_display=False, pre_hardware_hook_fn=hook_fn) as acq:
NameError: name 'Acquisition' is not defined

Missing import. Need to add from pycromanager import Acquisition at the top

@henrypinkard
Copy link
Member

When running one of the test scripts directly, them output is empty.

If the output is empty and the process completes, then that is a success.

ImportError while loading conftest 'C:\projects\personal\pycro-manager\pycromanager\test\conftest.py'.
conftest.py:6: in
import wget
E ModuleNotFoundError: No module named 'wget'

This is because there are extra requirements for setting up the test environment, because the test environment is running in GH actions and it needs to download the MM nightly build and latest dependencies. wget, for example, is used to do this downloading. If you already have it downloaded, you can ignore this, but it means you need to run code of the tests directly, rather than running through the whole thing and having try to set up the environment for you. If you want to run it through the full environment, the fix is to pip install all these extra requirements:

pip install pytest wget requests

@ieivanov plans to write out in more detail how it all works at some point micro-manager/pycro-manager#608

@nicost
Copy link
Member Author

nicost commented Jun 7, 2023

Cool!

That now runs, I added the test:
{'axes': {'time': 0}, 'exposure': 100.0, 'tags': {'test1': 'something'}},
and it runs correctly.

How do I run those files in the test directory, so that I can actually edit them and create a PR? Ah, sorry, those posts crossed. Without those details, I would not really ask others to contribute to the tests.

@ieivanov
Copy link
Contributor

ieivanov commented Jun 7, 2023

I'll get to documenting the testing infrastructure in the next couple of days, seems I've been putting it off long enough :)

I think this PR is a great addition. A couple of related issues:
keeping track of the position label in pycromanager datasets: micro-manager/pycro-manager#575
attaching arbitrary metadata to micromanager images: micro-manager/mmCoreAndDevices#329

@henrypinkard
Copy link
Member

How do I run those files in the test directory, so that I can actually edit them and create a PR?

I'm a bit confused--are you asking how do you run all the tests from an IDE like pycharm? I do this by just right clicking the file I'm changing and selecting run. Ivan just made this PR with more instructions, including the comand line way to run the tests pytest -v (this was news to me :))

@nicost
Copy link
Member Author

nicost commented Jun 9, 2023

Cool! That did not work earlier because of missing requirements. I still do not see the

pip install pytest wget requests

mentioned in the documentation. Am I missing something?

@henrypinkard
Copy link
Member

I believe this gets handled by pip install -e ".[dev]", which reads from the file listing all the extra dependencies needed for development

@ieivanov
Copy link
Contributor

ieivanov commented Jun 9, 2023

I believe this gets handled by pip install -e ".[dev]", which reads from the file listing all the extra dependencies needed for development

That's correct. I'll add instructions also on how to runs tests in PyCharm: https://www.jetbrains.com/help/pycharm/pytest.html#run-pytest-test and VSCode: https://code.visualstudio.com/docs/python/testing#_run-tests

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 a pull request may close this issue.

3 participants