mobie / mobie.github.io

1 stars 3 forks source link

Add "crop" to the MoBIE spec #44

Closed constantinpape closed 3 years ago

constantinpape commented 3 years ago

As discussed in #32 and https://github.com/bigdataprocessor/bigdataprocessor2/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

constantinpape commented 3 years ago

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 commented 3 years ago

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

constantinpape commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/mobie/mobie.github.io/issues/44#issuecomment-841416722.

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 commented 3 years ago

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 commented 3 years ago

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

:+1:

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 commented 3 years ago

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 commented 3 years ago

Sorry, here is the place that I meant.

constantinpape commented 3 years ago

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 commented 3 years ago

Ok, I made the following changes to the spec:

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

tischi commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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 commented 3 years ago

Works ❤️

image
martinschorb commented 3 years ago

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 commented 3 years ago

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

martinschorb commented 3 years ago

Hi,

I am trying to get a cropped bookmark to work as intended.

This is how it looks at the moment;

"crop-bookmark-002": {
      "isExclusive": true,
      "sourceDisplays": [
        {
          "imageDisplay": {
            "color": "white",
            "contrastLimits": [
              -127.0,
              127.0
            ],
            "name": "image-group-0",
            "opacity": 1.0,
            "sources": [
              "_KM140520_Grid3_c154"
            ]
          }
        }
      ],
      "sourceTransforms": [
        {
          "affine": {
            "parameters": [0.6013191061021975,
                -0.3915124093397475,
                -0.696515158463504,
                0.0,
                -0.3915124093397475,
                0.615527182232349,
                -0.683991462860532,
                0.0,
                0.696515158463504,
                0.683991462860532,
                0.2168462883345464,
                0.0],
            "sources": [
              "_KM140520_Grid3_c154"
            ]
          }

        }
      ],
            "crop": {
                "max": [0.54916642, 0.54917618, 0.82490016],
                "min": [-0.55025146, -0.5502417 , -0.27451772],
                "shiftToOrigin": true,
                "sourceNamesAfterTransform": [
                    "tomo1-cropped"
                ]
            },
            "viewerTransform": {
            "normalizedAffine": [1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0],
            "timepoint": 0
        },
      "uiSelectionGroup": "bookmark"
    }

This will crop the volume at the physical XYZ limits. However the cropping should happen in the viewer's coordinate frame to ensure all cropped bookmarks are rectangular, so they can be eventually be shown in a grid. How could I specify the boundaries in "Viewer coordinates"?

martinschorb commented 3 years ago

So I checked the specs and found out that the crop needs to live inside the sourceTransforms, otherwise this entry just seems to get ignored. Also it did not contain a sources target.

now it looks like:

"crop-bookmark-002": {
      "isExclusive": true,
      "sourceDisplays": [
        {
          "imageDisplay": {
            "color": "white",
            "contrastLimits": [
              -127.0,
              127.0
            ],
            "name": "image-group-0",
            "opacity": 1.0,
            "sources": [
              "_KM140520_Grid3_c154"
            ]
          }
        }
      ],
      "sourceTransforms": [
        {  "affine": {
            "parameters": [0.6013191061021975,
                -0.3915124093397475,
                -0.696515158463504,
                0.0,
                -0.3915124093397475,
                0.615527182232349,
                -0.683991462860532,
                0.0,
                0.696515158463504,
                0.683991462860532,
                0.2168462883345464,
                0.0],
            "sources": [
              "_KM140520_Grid3_c154"
            ]
          }
        },
                {
                    "crop": {
                    "max": [0.54916642, 0.54917618, 0.82490016],
                    "min": [-0.55025146, -0.5502417 , -0.27451772],
                    "shiftToOrigin": true,
                    "sourceNamesAfterTransform": [
                        "tomo1-cropped"
                    ],
                    "sources": [
                        "_KM140520_Grid3_c154"
                    ]
                }
            }
      ],
            "viewerTransform": {
            "normalizedAffine": [1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0],
            "timepoint": 0
        },
      "uiSelectionGroup": "bookmark"
    }

but still does not crop... If I put the crop entry before the affine, it does crop, but then the cropped volume block is not oriented with the viewer axes.

martinschorb commented 3 years ago

This now is the crop-bookmark-002 in the default dataset in /g/emcf/pape/mobie-test-projects

tischi commented 3 years ago

@martinschorb right now the crop is before the affine! I thought you wanted me to test it when you first affine transform and then crop?

martinschorb commented 3 years ago

right now the crop is before the affine

yes, I changed that now, but the behaviour is exactly the same. It looks like no matter where in the JSON the crop appears, it is applied first.

BTW: Does your viewer also flicker a lot when scrolling through this dataset?

Martin

martinschorb commented 3 years ago

I also now specify the source names using "sourceNamesAfterTransform" in the affine step and then refer to this source name during the crop. No success...

martinschorb commented 3 years ago
Screenshot 2021-07-05 at 13 02 15

I often get this stripy appearance with frayed edges.

martinschorb commented 3 years ago

the black fringes are probably due to zero-value misbehaviour.

https://github.com/bigdataviewer/bigdataviewer-core/issues/126

tischi commented 3 years ago

@martinschorb There were some issue that I fixed (uploaded to MoBIE-beta already). However, the main issue is that the viewerTransform is wrong. If you press shift+Z you will see that the cropped region is in fact aligned with the xyz axes of the global coordinate system.

image

If, after you do this, you right click into the BDV window and select "BDV - Log current view" you get:

{"normalizedAffine":[0.19159879848220396,-0.12474791913167561,0.22193119448888496,-0.17207452938728296,-0.12474791913167561,0.19612592939764514,0.21794075911815444,-0.18462491577570794,-0.22193119448888496,-0.21794075911815444,0.06909391016949239,-0.001390719187258005],"timepoint":0}

That means the correct viewerTransform would be: [0.19159879848220396,-0.12474791913167561,0.22193119448888496,-0.17207452938728296,-0.12474791913167561,0.19612592939764514,0.21794075911815444,-0.18462491577570794,-0.22193119448888496,-0.21794075911815444,0.06909391016949239,-0.001390719187258005]

Could you test what you get if you don't specify any viewerTransform? It may look better.

martinschorb commented 3 years ago

Hi,

without specifying a viewer transform, it looks tho me that neither the affine nor the crop are applied at all. I gave some very strange crop limits to test and it still looks as if it is showing the entire volume (= the first bookmark). (After updating to the newest Beta)

tischi commented 3 years ago

And in my screenshot above, does that look correct?

tischi commented 3 years ago

Ok, I agree, it looks as if it is the whole image after the crop.

Could you do me a favor and add another bookmark where you only crop?

tischi commented 3 years ago

It would also be good to have a crop region that is reasonable, maybe half the size of the original image. The current one give super weird results for me: [(0.0, 0.0, 0.0) -- (0.001, 0.001, 300000.0)]

tischi commented 3 years ago

I saw that I have write access, so I changed it myself.

tischi commented 3 years ago

@martinschorb I think it was simply a matter of choosing a crop interval that actually is within the image after you applied the affine transformation:

"crop": {
                    "max": [0.4, -0.3, 1.6],
                    "min": [0.3, -0.4, 1.0],

Yields this:

image
martinschorb commented 3 years ago

OK, that makes sense. Still the affine is not applied here (the volume is shown in its own XYZ cordinates) and that's why my calculated crop boundaries do not work.

Let's take a step all the way back:

Is there a conceptual mistake on my side as to where the transforms should be applied? Constantin suggested to force the viewer to the original axes using viewerTransform. Or is the underlying problem that there is only one active source and that's why BDV will always adapt to this coordinate system?

tischi commented 3 years ago

Short zoom? I'll send a link in a mail.

tischi commented 3 years ago

@martinschorb

opening crop-bookmark-imported I am getting this:

image

The width of the crop is about 700 nm.

  1. Is this correct? If not or you are not sure, could you please post a screenshot of what you expect?
  2. Could you please put two crops there with a grid view on top?
martinschorb commented 3 years ago

Hi, this does not really look correct. I have now created a proper repository for this MoBIE project with all data in

/g/schwab/Tobias/MoBIE

I am right now trying to generate one example crop that we can use to check. Give me 3 minutes...

tischi commented 3 years ago

Take your time. I will check after lunch ;-) It would be very helpful if you posted a screenshot here of what it should look like.

martinschorb commented 3 years ago

the dataset.json is there now.

It should look more or less like this:

Screenshot 2021-07-21 at 12 21 29
martinschorb commented 3 years ago

@constantinpape in case you want to reproduce:

infile = 'Grid3_c550'
xlfile = './Tabellen/CTRL/KM020720_measures_raw1.xlsx'

gives me the transformation and crop limits that I manually put into the JSON.

tischi commented 3 years ago

@martinschorb I have trouble mounting /g/schwab; is it just me?