Add "crop" to the MoBIE spec #44

constantinpape · 2021-05-10T13:09:45Z

As discussed in #32 and bigdataprocessor/bigdataprocessor2#168, it would be good to express a "crop" in the MoBIE spec.
I think the best option would be to add this as an additional source transform and/or viewer transform.
Let me know what you think, @tischi @martinschorb @K-Meech.

tischi · 2021-05-10T13:48:34Z

additional source transform

That sounds very good. And then I would suggest it is users responsibility to first add a source transform to, e.g. rotate, the source into a space where the crop can be expressed in Cartesian coordinates. We could think of adding functionality to, e.g. BigDataViewer playground or BigDataProcessor2, that makes it easy for the user to determine both the transform and the crop. (ping @NicoKiaru).

constantinpape · 2021-05-11T07:28:38Z

That sounds very good.

Ok, should we go with:

{"crop": {"start": [0.0, 0.0, 0.0], "stop": [10.1, 11.2, 12.3]}}

?
Any other preferences?

And then I would suggest it is users responsibility to first add a source transform to, e.g. rotate, the source into a space where the crop can be expressed in Cartesian coordinates.

Exactly.

tischi · 2021-05-11T08:30:08Z

start and stop sounds a bit funny 😄
I would prefer min and max.

constantinpape · 2021-05-11T08:37:54Z

start and stop sounds a bit funny

That's what it's called in python: https://www.programiz.com/python-programming/methods/built-in/slice
But I am fine with min and max.

martinschorb · 2021-05-11T11:07:27Z

As a consequence this functionality would enable nicely automated galleries of bookmarks.

the bookmarks would define the view transformation and center of the crop
you would then only need to provide the size of the crop in physical units
this would then fully define everything necessary for a gallery of all features in a set of bookmarks, regardless of whether they are located in a single source or multiple sources

constantinpape · 2021-05-11T14:21:25Z

Ok, I have added the crop transform to the spec now.
It is defined like this:

{
  "crop": {
    "min": [0.0, 0.0, 0.0],
    "max": [1.1, 2.2, 3.3],
    "sources": ["mySourceNames"],
    "timepoints": [0, 1, 2]
  }
}

The timepoints field is optional; if it is missing the transform will be applied to all timepoints.

martinschorb · 2021-05-13T06:34:51Z

What would you think would be the best strategy to make the cropped sources available in MoBIE? Would you list them as extra 'images'? or 'bookmarks'?

I think it would also be beneficial to give each crop a 'name' or ID to be able to identify them (for example when used in a gallery).

constantinpape · 2021-05-14T15:15:01Z

What would you think would be the best strategy to make the cropped sources available in MoBIE? Would you list them as extra 'images'? or 'bookmarks'?

This depends on what you want to achieve. If it makes sense to look at the cropped source independently, then it can be provided as an extra image. If not, it should only be added to the bookmarks.
But this is up to whoever creates the MoBIE project; it's possible to express both options in the current spec (once crop is implemented in the fiji viewer).

I think it would also be beneficial to give each crop a 'name' or ID to be able to identify them (for example when used in a gallery).

I think that's the case anyway; each source in the view will be listed. But let's see how this looks once we have the crop functionality in the viewer; I think it's much easier to discuss this when we have a working example already.

martinschorb · 2021-05-14T15:37:21Z

I think that's the case anyway; each source in the view will be listed.

One source can have multiple crop regions. Hence I see the need for an independent tag for those. Or would you suggest to define multiple sources from the same image but pointing to different crop regions?

tischi · 2021-05-14T17:07:59Z

Or would you suggest to define multiple sources from the same image but pointing to different crop regions?

Currently that's what one would need to do I think as the transformations do not change the name of the sources. Adding them as an extra sources with the cropped default view to the dataset.json also could allow you to give them a name of your liking. I thus think that could be a good mechanism.

However, I think the current spec requires that name of the source in dataset.json MUST be the same as the name in the xml. We would need to allow for them to be different in order to add the same source several times with different crops, because the names of the sources in dataset.json MUST be unique.

tischi · 2021-05-14T17:40:56Z

Frankly, above comment was just a knee-jerk reaction 😄

Second thought is that this is not a good idea.

The sources should just define the "raw" sources and any special "view" on them (such as a crop) should very likely be defined in the views.

constantinpape · 2021-05-14T18:13:45Z

We could add an optional name to the source transforms. If there is no such name, the UI shows the source name. If the name is specified in the transform, this name is used. This introduces some corner cases, because multiple names could be specified for one source via multiple transforms. In that case, we could either throw a runtime error, or just use the last or first name.

tischi · 2021-05-15T08:11:44Z

We could add an optional name to the source transforms.

I think maybe that issues solves itself when we do this? Because then the name is automatically determined by the view that applies the transformation(s).

tischi · 2021-05-15T09:04:45Z

I think we may need another field in the crop, namely shiftToOrigin specifying whether after the crop the region should be moved to (true) (0,0,0) or whether it should (false) stay in physical space where it was. I think we need both. true in order to overlay onto other sources, false in order to generate a gallery using the grid (where currently the sources are shifted relative to their current position in space).

constantinpape · 2021-05-15T09:15:07Z

I think maybe that issues solves itself when we do this? Because then the name is automatically determined by the view that applies the transformation(s).

While I agree that we should do the change you proposed in #46 I don't think it solves the particular issue here:
the issue is that we can have multiple transforms of the same source in a single (grid) view, for example the same source with different crops applied to it. Now we need different names to distinguish this. The only solution I see here is to add an optional name to sourceTransforms: #44 (comment).

I think we may need another field in the crop, namely shiftToOrigin specifying whether after the crop the region should be moved to (true) (0,0,0) or whether it should (false) stay in physical space where it was. I think we need both. true in order to overlay onto other sources, false in order to generate a gallery using the grid (where currently the sources are shifted relative to their current position in space).

Makes sense and I can add it. Do you think this should be mandatory or optional? If optional, what would be the default? I think true?!

tischi · 2021-05-16T11:40:00Z

the issue is that we can have multiple transforms of the same source in a single (grid) view, for example the same source with different crops applied to it

Good point! Let's add an optional name to the transform. I guess the point would be to append this to the source name?

Do you think this should be mandatory or optional?

Yes, let's do optional with default true. Is "shiftToOrigin" the best name for the field?

constantinpape · 2021-05-16T13:12:47Z

Good point! Let's add an optional name to the transform. I guess the point would be to append this to the source name?

👍

Yes, let's do optional with default true. Is "shiftToOrigin" the best name for the field?

Not sure; I can't think of anything better right now.

tischi · 2021-05-16T14:37:05Z

I am now not so sure about whether we should append the name.
The point is that one has to exactly match the name of the transformed (e.g. cropped) source when referring to it in a grid view.
Thus one would need to know how exactly MoBIE appends (e.g. space or dash separated).
Currently I feel it is easier to fully replace the name (this also avoids the issue that names could get very long).

tischi · 2021-05-16T14:38:20Z

Sorry, here is the place that I meant.

constantinpape · 2021-05-16T17:14:15Z

I am now not so sure about whether we should append the name.
The point is that one has to exactly match the name of the transformed (e.g. cropped) source when referring to it in a grid view.
Thus one would need to know how exactly MoBIE appends (e.g. space or dash separated).
Currently I feel it is easier to fully replace the name (this also avoids the issue that names could get very long).

I am also not sure what's the best solution here. I see the issues you bring up with append.
The problem with replace however is that this does not work when a transform is applied to multiple sources, because they all would end up with the same name.

At least for the issue of how names are concatenated, we could say that MoBIE does not introduce any separators, so that this is up to the user.

constantinpape · 2021-05-16T18:15:23Z

Ok, I made the following changes to the spec:

"crop" has "shiftToOrigin", which should default to true.
All source transforms support an optional "name" field. For now, I have written in the description that this name will be appended to the source name.

@tischi let me know if you want to change any of this.

tischi · 2021-05-17T07:44:41Z

I am thinking now that the "name" should maybe not be the name of the transform but rather of the transformed source(s). Thus it would be "names", being a list of the same length as the "sources".

I am favoring "replace", because a convenience append mode could be implemented in the software that create the mobie projects (e.g. your python package).

constantinpape · 2021-05-17T07:50:31Z

I am thinking now that the "name" should maybe not be the name of the transform but rather of the transformed source(s). Thus it would be "names", being a list of the same length as the "sources".

Yes, I agree that's much better. I will change it.

constantinpape · 2021-05-17T08:54:40Z

Ok, I updated it. To summarize all the relevant changes:

We support the source transform "crop" now.
All source transforms ("crop", "affine", "grid") have a new optional field "names", which allows to replace the source names after transformation.

Here's an example of the crop transform:

"sourceTransforms": [
  {
    "crop": {
      "min": [0.0, 0.3, 2.1],
      "max": [1.0, 2.3, 3.9],
      "sources": ["imageA", "imageB"],
      "names": ["imA-cropped", "imB-cropped"],
      "shiftToOrigin": true
    }
  }
]

A full project with a crop transform can be found here:
/g/emcf/pape/mobie-test-projects/mobie_crop.
I also added the new "names" feature here. And added another bookmark to test the "shiftToOrigin" feature.

tischi · 2021-05-17T14:32:26Z

Works ❤️

martinschorb · 2021-06-14T10:52:08Z

Cool!

I finally found the time to look into that again.
Now, I'd like to combine crop with affine and eventually grid to get the final view and I am wondering what the effect of

 "shiftToOrigin": true

will be depending on when the crop happens will be.

martinschorb · 2021-06-14T11:57:22Z

An affine after a crop seems to have no effect at all.
But a crop executed last gives good results.

tischi · 2021-07-21T12:17:27Z

@martinschorb

for debugging: could you please also add a bookmark where you only do the transformation without the crop?

because then we can look there whether the crop bounding box makes sense.

martinschorb · 2021-07-21T12:23:06Z

Yep that's in there already.

tischi · 2021-07-21T12:25:23Z

Yes, just saw that!

But I think you have a bug because in the crop you are not referring to the correct name of the transformed source:

"sourceTransforms": [
        {  "affine": {
            "parameters": [-0.32217264,  0.93204427, -0.16582599,  0.        , -0.93204427,
	       -0.28160441,  0.22801848,  0.        ,  0.16582599,  0.22801848,
	        0.95943177,  0.        ],
						"sourceNamesAfterTransform": [
	  						"tomo1-transformed2"
	  					],
            "sources": [
              "_KM020720_Grid3_c550"
            ]
          }
        },
				{
					"crop": {
					"max": [ 1.36148167, -1.32476456,  1.47788611],
					"min": [ 0.30069031, -2.38555591,  0.41709475],
					"shiftToOrigin": true,
					"sourceNamesAfterTransform": [
						"tomo1-cropped"
					],
					"sources": [
						"tomo1-transformed"
					]
				}
			}

tischi · 2021-07-21T12:25:56Z

I fixed it for you.

tischi · 2021-07-21T12:28:07Z

That's how the crop looks like; similar to yours but wider:

tischi · 2021-07-21T12:29:32Z

oops, just realizing this looks like the raw data...

tischi · 2021-07-21T12:39:05Z

@constantinpape @martinschorb
There is one issue: the source names in the json and within the xml do not match, e.g.:

xml:

      <ViewSetup>
        <id>0</id>
        <name>c515</name>
        <size>2024 2024 769</size>

json:

_KM270121pos_Grid4_c515

Currently, my code expects the source names in the dataset.json to be identical to the source names in the xml. Is that what we also require in the specification?!

constantinpape · 2021-07-21T12:42:57Z

Is that what we also require in the specification?!

Yes, we also require it in the spec. @martinschorb, you will need to fix it in your code that writes the data, this pybdv function is useful here: https://github.com/constantinpape/pybdv/blob/master/pybdv/metadata.py#L843

tischi · 2021-07-21T12:44:24Z

Renaming the original sources to other names than what is specified in the xml is something we could implement but I would like to avoid it because we would loose compatibility with the current bdv-playground, see this issue: bigdataviewer/bigdataviewer-playground#186

tischi · 2021-07-21T12:45:07Z

@martinschorb is it OK for you if I just manually change the name of the cropped source in the xml such that I can proceed with the testing?

tischi · 2021-07-21T12:46:29Z

I just did it 😸 , renamed source in: bdv-n5/_KM020720_Grid3_c550.xml

tischi · 2021-07-21T12:51:01Z

@martinschorb I think in principle it is working:

You (we) still may have to play a bit with the transformation to orient the centriole(?) in the crop bounding box, but at least it seems to be nicely within the crop.

martinschorb · 2021-07-21T13:01:43Z

cool.

The goal would be that it is oriented along the x axis. At least that is what the linear algebra is supposed to do...

martinschorb · 2021-07-21T13:02:17Z

I have to take care of a 5th year Kindergeburtstag now. I'm curious how it proceeds...

tischi · 2021-07-21T13:08:01Z

I think I cannot do anything from my side right now.

Maybe you could just fix above issues and add more of those crops and arrange them in a grid view and then we take it from there.

martinschorb · 2021-07-21T13:41:54Z

Is it in a state that you can make it available via the beta update site? Or can you provide me a compiled jar? that would be very helpful so I can play with the transforms.

tischi · 2021-07-21T14:45:21Z

I just pushed the latest master to MoBIE-beta (ping @K-Meech @KateMoreva)

martinschorb · 2021-07-22T05:48:17Z

Looks good.
I need to work a bit on the transforms (flipping the rotation matrix looks very promising...)

martinschorb · 2021-07-22T06:19:52Z

Nailed it!

second coordinate axes needs to be treated inverted as always when dealing with MRC format...

I will now try to incorporate the JSON generation into my script and then we are ready.

martinschorb · 2021-07-22T06:26:04Z

One general thing we need to consider:

how do we deal with varying crop sizes?

These objects have all varying lengths.
Since the gallery is displayed in one BDV window, the scale is the same for all of them.
If I dynamically adjust the crop size according to object size, the gallery tiles will have different sizes.

This probably looks terrible...

I can see a couple of options:

we use a fixed crop size, with the risk of cutting off some interesting parts of long objects
we try to find the maximum object length of the dataset and make the tiles that size (risk of having many almost "empty" tiles for small objects)
we use varying tile sizes and the gallery constructor takes care of ordering them properly so the gallery still looks OK.

How would you go about it?

martinschorb · 2021-07-22T06:33:17Z

@constantinpape

cropvals and vec2mat (in https://github.com/mobie/MM_centrioles-tomo-datasets/blob/main/centriole_analysis.py) will give you the transformation and crop intervals for any given object specified by the start end end vector in voxel space. Maybe that is useful for other projects as well.

tischi · 2021-07-22T07:11:12Z

we use varying tile sizes and the gallery constructor takes care of ordering them properly so the gallery still looks OK.

I would say lets go for this option. We already discussed at some point (and I cannot find the issue) that some optimal space filling algorithm for 2D tiles of different sizes could be a lot of fun to implement.

constantinpape · 2021-07-22T07:27:49Z

cropvals and vec2mat (in https://github.com/mobie/MM_centrioles-tomo-datasets/blob/main/centriole_analysis.py) will give you the transformation and crop intervals for any given object specified by the start end end vector in voxel space.

Cool, I will check it out.

we use varying tile sizes and the gallery constructor takes care of ordering them properly so the gallery still looks OK.

I would say lets go for this option. We already discussed at some point (and I cannot find the issue) that some optimal space filling algorithm for 2D tiles of different sizes could be a lot of fun to implement.

Yes, I would also vote for trying this first. I also couldn't find this issue, but it's easy to google: https://en.wikipedia.org/wiki/Rectangle_packing
(and it's important for website rendering, so there should be some good reference implementations)

martinschorb · 2021-07-26T16:17:36Z

@tischi it stopped working with my crops (/g/schwab/Tobias/MoBIE) You can see the viewer being oriented correctly for a glimpse of a second but it then jumps back to the default view of the source...

Tried with the current beta on Mac and Windows.

constantinpape · 2021-07-26T19:30:10Z

@martinschorb maybe this is because you have added sourceNamesAfterTransformation now; I think we haven't fully tested this yet.

martinschorb · 2021-07-27T05:57:44Z

sourceNamesAfterTransformation

I head this in earlier versions as well (see dataset_manual.json from Friday) and it worked. I don't see what could have changed. There were no updates in Fiji since it worked. The only thing that changed is the dataset.json whose entries look the same to me, and the chunk size in the linked n5, but this should be totally irrelevant.

martinschorb · 2021-07-27T06:08:46Z

OK, found it. It was the "old" problem that the sourceNamesAfterTransformation was missing for the affine trafos.

martinschorb · 2021-07-27T06:25:44Z

Looks like it works.

But I cannot close this issue...

tischi · 2021-07-27T06:38:59Z

Nice that it works! I will close it.

constantinpape mentioned this issue May 10, 2021

crop in transformed coordinates bigdataprocessor/bigdataprocessor2#168

Open

constantinpape mentioned this issue May 11, 2021

Proposal: structure of a source-pairwise registration definition #32

Open

martinschorb mentioned this issue Jun 21, 2021

Problems with example project with crop mobie/mobie-viewer-fiji#313

Closed

tischi closed this as completed Jul 27, 2021

Add "crop" to the MoBIE spec #44

Add "crop" to the MoBIE spec #44

Comments

constantinpape commented May 10, 2021

tischi commented May 10, 2021

constantinpape commented May 11, 2021

tischi commented May 11, 2021

constantinpape commented May 11, 2021

martinschorb commented May 11, 2021

constantinpape commented May 11, 2021

martinschorb commented May 13, 2021

constantinpape commented May 14, 2021

martinschorb commented May 14, 2021

tischi commented May 14, 2021

tischi commented May 14, 2021

constantinpape commented May 14, 2021

tischi commented May 15, 2021

tischi commented May 15, 2021

constantinpape commented May 15, 2021

tischi commented May 16, 2021

constantinpape commented May 16, 2021

tischi commented May 16, 2021

tischi commented May 16, 2021

constantinpape commented May 16, 2021

constantinpape commented May 16, 2021

tischi commented May 17, 2021

constantinpape commented May 17, 2021

constantinpape commented May 17, 2021 • edited Loading

tischi commented May 17, 2021

martinschorb commented Jun 14, 2021

martinschorb commented Jun 14, 2021

tischi commented Jul 21, 2021

martinschorb commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

constantinpape commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

tischi commented Jul 21, 2021

martinschorb commented Jul 21, 2021

martinschorb commented Jul 21, 2021

tischi commented Jul 21, 2021

martinschorb commented Jul 21, 2021

tischi commented Jul 21, 2021

martinschorb commented Jul 22, 2021

martinschorb commented Jul 22, 2021

martinschorb commented Jul 22, 2021

martinschorb commented Jul 22, 2021

tischi commented Jul 22, 2021

constantinpape commented Jul 22, 2021

martinschorb commented Jul 26, 2021

constantinpape commented Jul 26, 2021

martinschorb commented Jul 27, 2021

martinschorb commented Jul 27, 2021

martinschorb commented Jul 27, 2021

tischi commented Jul 27, 2021

constantinpape commented May 17, 2021 •

edited

Loading