mobie / mobie-viewer-fiji

BSD 2-Clause "Simplified" License
29 stars 12 forks source link

Defaults for centerAtOrigin #679

Closed tischi closed 2 years ago

tischi commented 2 years ago

Double check the default of centerAtOrigin for all implementations of: AbstractSourceTransformer

Somehow I remember that, for instantiation from JSON, one needs to do more than just set to some value in the field...

@constantinpape what do we say is the default in the spec?

constantinpape commented 2 years ago

The default is true.

Somehow I remember that, for instantiation from JSON, one needs to do more than just set to some value in the field...

Yes, I also remember that there was something that needed to be changed, not 100% sure what exactly...

constantinpape commented 2 years ago

btw, the default I refered to was for crop. There it should be true. But if for the transformedGrid it makes more sense to set it to false that would be fine with me, see https://github.com/mobie/covid-if-project/issues/2#issuecomment-1107546099.

tischi commented 2 years ago

we have to decide for those four:

image
tischi commented 2 years ago

It is a bit complicated: For the first grid I think you always want true => default true seems good in general. But if you then do second grid of a grid, it depends: for TransformedGrid you then want false, otherwise you delete the effects of the first grid; however for a MergedGrid of a MergedGrid, you may want true again, because the output of the first MergedGrid are new merged sources...

constantinpape commented 2 years ago
  • AffineSourceTransformer does not have centerAtOrigin, which probably makes sense?

Yes, doens't need it.

  • CropSourceTransformer: true?!

  • MergedGridSourceTransformer: true?!

  • TransformedGridSourceTransformer: true and then false?!

Ok, I would then leave it as true for all cases, and for the more transformedGrid it needs to be done correctly by whoever creates the project -- I think having different default values depending on nesting grids gets to complex and should be the user's responsibility.

tischi commented 2 years ago

I checked, and also the MergedGridSourceTransformer needs to have the centerAtOrigin set to false when operating on already transformed sources. Thus, it seems as if both grid transformers have the same behavior here (first run: true, second run: false)

tischi commented 2 years ago

Ok, I would then leave it as true for all cases, and for the more transformedGrid it needs to be done correctly by whoever creates the project -- I think having different default values depending on nesting grids gets to complex and should be the user's responsibility.

Ok! Could you then change the default view in covid-if accordingly for testing this? Currently also the second round of grid building uses centerAtOrigin=true:

Merged 9 sources into E06_nuclei in 34ms (centerAtOrigin=true).
Merged 9 sources into E07_nuclei in 4ms (centerAtOrigin=true).
Merged 9 sources into E06_serumIgG in 3ms (centerAtOrigin=true).
Merged 9 sources into E07_serumIgG in 2ms (centerAtOrigin=true).
Merged 9 sources into E06_marker_tophat in 2ms (centerAtOrigin=true).
Merged 9 sources into E07_marker_tophat in 3ms (centerAtOrigin=true).
Merged 9 sources into E06_cell_segmentation in 2ms (centerAtOrigin=true).
Merged 9 sources into E07_cell_segmentation in 4ms (centerAtOrigin=true).
Merged 9 sources into E06_nucleus_segmentation in 4ms (centerAtOrigin=true).
Merged 9 sources into E07_nucleus_segmentation in 2ms (centerAtOrigin=true).
Merged 2 sources into plate_nuclei in 4ms (centerAtOrigin=true).
Merged 2 sources into plate_serumIgG in 6ms (centerAtOrigin=true).
Merged 2 sources into plate_marker_tophat in 3ms (centerAtOrigin=true).
Merged 2 sources into plate_cell_segmentation in 1ms (centerAtOrigin=true).
Merged 2 sources into plate_nucleus_segmentation in 3ms (centerAtOrigin=true).
constantinpape commented 2 years ago

mergedGrid should default to false, but you probably have this already, @tischi. I think this is all done now, so I am closing the issue, feel free to reopen if anything is left to do here.