-
Notifications
You must be signed in to change notification settings - Fork 32
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
Allow a caller to ignore row numbers #418
Conversation
For calls to readCoordinates, read, and slice the returned value order in the `Data` response is the same as requested. While having the row numbers included in the response is convenient, when the number of cells being returned is high this incurs memory and serialization overhead. This is especially true when retrieving a small number of columns for a large number of rows; in this case `rowNumbers` can actually be more expensive to include than the data itself. Here we use the Ice context and "omero.tables.include_row_numbers" to additively affect the client API without changing any of the Ice method prototypes.
Example when used on a small table:
If we're happy with the implementation I'll make separate PRs to add integration tests like we have for the bitmask query and update the main OMERO.tables documentation detailing the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I'm surprised that the row numbers are longer than other columns except perhaps bools 😏 but I can definitely see how they would effectively double the overhead.
Also true for short string columns. When it comes to memory usage in Python in particular, also true where the same numbers or strings repeat. These are both common in a lot of the data analysis outputs we're exposed to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and functional testing against merge-ci server worked as described (default behaviour unchanged etc).
Integration test added in ome/openmicroscopy#6396. |
Documentation added in ome/omero-documentation#2441. |
Repository: ome/openmicroscopy Excluded PRs: - PR 6379 jburel 'TMP: build david's s3 Zarr branch' (exclude comment) - PR 6319 dominikl 'Add integration test for joining session' (exclude comment) - PR 6275 joshmoore 'Add OMERO5.4__1 for omero-model#71' (exclude comment) - PR 6212 jburel 'add test to import images' (exclude comment) - PR 6086 manics 'Alternative JSON configuration system for OMERO.web' (label: exclude) Already up to date. Merged PRs: - PR 6309 jburel 'add try block' - PR 6353 jburel 'remove dependencies' - PR 6376 snoopycrimecop 'update dependencies' - PR 6385 jburel 'remove test using deprecated method' - PR 6388 sbesson 'Upgrade Ivy to 2.5.2' - PR 6389 chris-allan 'Handle archive status being populated (See ome/omero-web#555)' - PR 6393 sbesson 'Add documentation for the omero.logging properties' - PR 6395 sbesson 'Update IceGrid templates to make the OMERO.tables module configurable' - PR 6396 chris-allan 'Add a test case for ome/omero-py#418' Generated by OMERO-push#112 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-push/112/)
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' Generated by OMERO-docs#113 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/113/)
Repository: ome/openmicroscopy Excluded PRs: - PR 6379 jburel 'TMP: build david's s3 Zarr branch' (exclude comment) - PR 6319 dominikl 'Add integration test for joining session' (exclude comment) - PR 6275 joshmoore 'Add OMERO5.4__1 for omero-model#71' (exclude comment) - PR 6212 jburel 'add test to import images' (exclude comment) - PR 6086 manics 'Alternative JSON configuration system for OMERO.web' (label: exclude) Already up to date. Merged PRs: - PR 6309 jburel 'add try block' - PR 6353 jburel 'remove dependencies' - PR 6376 snoopycrimecop 'update dependencies' - PR 6385 jburel 'remove test using deprecated method' - PR 6388 sbesson 'Upgrade Ivy to 2.5.2' - PR 6389 chris-allan 'Handle archive status being populated (See ome/omero-web#555)' - PR 6393 sbesson 'Add documentation for the omero.logging properties' - PR 6395 sbesson 'Update IceGrid templates to make the OMERO.tables module configurable' - PR 6396 chris-allan 'Add a test case for ome/omero-py#418' Generated by OMERO-push#113 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-push/113/)
Repository: ome/ome-documentation Excluded PRs: - PR 2443 snoopycrimecop 'Changes from upstream repositories: openmicroscopy' (status: failure) - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' - PR 2442 jburel 'Debian10' Generated by OMERO-docs#114 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/114/)
Repository: ome/openmicroscopy Excluded PRs: - PR 6379 jburel 'TMP: build david's s3 Zarr branch' (exclude comment) - PR 6319 dominikl 'Add integration test for joining session' (exclude comment) - PR 6275 joshmoore 'Add OMERO5.4__1 for omero-model#71' (exclude comment) - PR 6212 jburel 'add test to import images' (exclude comment) - PR 6086 manics 'Alternative JSON configuration system for OMERO.web' (label: exclude) Already up to date. Merged PRs: - PR 6309 jburel 'add try block' - PR 6353 jburel 'remove dependencies' - PR 6376 snoopycrimecop 'update dependencies' - PR 6385 jburel 'remove test using deprecated method' - PR 6388 sbesson 'Upgrade Ivy to 2.5.2' - PR 6389 chris-allan 'Handle archive status being populated (See ome/omero-web#555)' - PR 6393 sbesson 'Add documentation for the omero.logging properties' - PR 6395 sbesson 'Update IceGrid templates to make the OMERO.tables module configurable' - PR 6396 chris-allan 'Add a test case for ome/omero-py#418' Generated by OMERO-push#114 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-push/114/)
Repository: ome/ome-documentation Excluded PRs: - PR 2443 snoopycrimecop 'Changes from upstream repositories: openmicroscopy' (status: failure) - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' - PR 2442 jburel 'Debian10' Generated by OMERO-docs#115 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/115/)
Add a test case for ome/omero-py#418
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' - PR 2444 snoopycrimecop 'Changes from upstream repositories: openmicroscopy' Generated by OMERO-docs#116 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/116/)
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' Generated by OMERO-docs#117 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/117/)
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' Generated by OMERO-docs#118 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/118/)
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' Generated by OMERO-docs#119 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/119/)
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' Generated by OMERO-docs#120 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/120/)
Repository: ome/ome-documentation Excluded PRs: - PR 2432 ehrenfeu 'Some suggestions on the OMERO.movie page' (user: ehrenfeu) - PR 2393 bernt-matthias 'fix option capitalization' (user: bernt-matthias) - PR 2357 fmeyenhofer 'Add a OMERO.table visualisation example' (user: fmeyenhofer) - PR 2347 jburel 'pin version to match rtd ones' (exclude comment) - PR 2298 pwalczysko 'test' (exclude comment) Already up to date. Merged PRs: - PR 2291 pwalczysko 'Add ansible and docker install doc - former "install.rst"' - PR 2353 jburel 'Mencoder' - PR 2355 will-moore 'Update python Tables code to use 'Image' column' - PR 2424 DavidStirling 'Clarify CSRF token usage' - PR 2430 jburel 'exclude trac' - PR 2441 chris-allan 'Add documentation for ome/omero-py#418' Generated by OMERO-docs#121 (https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-docs/121/)
Add documentation for ome/omero-py#418
For calls to readCoordinates, read, and slice the returned value order in the
Data
response is the same as requested. While having the row numbers included in the response is convenient, when the number of cells being returned is high this incurs memory and serialization overhead. This is especially true when retrieving a small number of columns for a large number of rows; in this caserowNumbers
can actually be more expensive to include than the data itself.Here we use the Ice context and "omero.tables.include_row_numbers" to additively affect the client API without changing any of the Ice method prototypes.
/cc @erindiel, @kkoz, @DavidStirling, @emilroz