Merge populate_metadata PRs (rebased onto develop) #5241
Conversation
|
--rebased-from #5232 |
Most methods for dataset loading and parsing were left unimplement. Now a `Dataset:`-style object can be passed to populate_metadata.py and images will be looked up by name. Note: there's a small bug with name lookup that will be corrected separately.
The assumptions for well/imaging naming in a plate or screen differ from those from image naming in a dataset since there's no unique way to reference an image in a dataset like there is well "A1" for example. This commit loosens some of those rules to allow image columns and image name columns to work together in the case of datasets. The assumption is that for population the ID of the image in a dataset won't be known. Instead names of images will be used as a unique identifier. Currently only a warning is issued if the name is not unique.
In general, populate_metadata.py looks to be in line for a refactoring. The number of if-clauses as well as the unhandled cases (like no catch-all for unknown targets in delete) is making this ever harder to work with. All tests passing.
In order to allow Projects to smartly handle multiple images with the same name (though not in the same dataset), the internals of ValueResolver have been hidden within a ValueWrapper class. ValueResolver chooses once which ValueWrapper to use internally after which the various if/then blocks based on target object are no longer necessary (needs further refactoring). There *are* still if/then blocks basked on column-type. These could use some cleaning but will likely remain to be necessary for multiple-dispatch style handling.
For extremely large screens (idr0016), both adding map annotations as well as deleting them lead to either PG errors or Ice.MessageSizeMax exceptions. Now both are done in batches of 1000.
| def createCsv(self, *args, **kwargs): | ||
| csvFileName = super(GZIP, self).createCsv(*args, **kwargs) | ||
| gzipFileName = "%s.gz" % csvFileName | ||
| with open(csvFileName, 'rb') as f_in, \ |
There was a problem hiding this comment.
There was a problem hiding this comment.
@aleksandra-tarkowska Maybe try nesting them?
with open(csvFileName, 'rb') as f_i:
with gzip.open(gzipFileName, 'wb') as f_out:
|
Quite a few test failures with yaml issues at https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-integration-Python27/523/testReport/ |
|
PASS https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-Python27/528/testReport/ FAILED: https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-python/554/testReport/ that test should be excluded |
|
|
||
| @mark.skipif(sys.version_info < (2, 7), | ||
| reason="requires python2.7") | ||
| class TestPopulateMetadata(TestPopulateMetadataHelper): |
There was a problem hiding this comment.
why that is not skipped?
There was a problem hiding this comment.
Does it detect the error by static analysis without having to even run the test?
There was a problem hiding this comment.
My reading of https://docs.pytest.org/en/latest/skipping.html#id1 is that the function (or class in that case) is initialized and then the abortion is testing when setting up the tests. Here, because there is some Python 2.7 specific bit in the static class variables, the class initialization fails before it has a chance to execute the skipping logic.
|
Thanks, @aleksandra-tarkowska! Great to have this making it back to the mainline. Merging for 5.3.2 after discussion with @jburel. |
This is the same as gh-5232 but rebased onto develop and gh-5243
What this PR does
merge #5218 #5220 #5222 #5223 #5224 #5226 #5227 #5229 (no conflicts)
TODO: