Geometric transformations on entire object

[Nick-Eagles]

trafficstars

Hello,

I ran into a case where I found it useful to apply rotations, reflections, and translations to the entire SpatialExperiment object. So far, I just wrote code to adjust the spatialCoords following these transformations, since I noticed similar code already existed to adjust just the imgData slot (https://github.com/drighelli/SpatialExperiment/blob/a4d45fd86b64bcaf24d740e9266e7e9a64888367/R/imgData-methods.R#L235-L259). If you think geometric transformations that affect both the spatialCoords and imgData would be useful functionality, I'd be happy to do a pull request. I'm not yet familiar with writing code that adheres to required Bioconductor package syntax, but perhaps just providing what I have might still be a useful start.

Let me know if a pull request sounds appropriate-- otherwise, feel free to close this issue.

Thanks, -Nick

[lcolladotor]

Hi!

We'd love to submit a PR about this functionality since we think that it would be a good companion to the rotateImg() and mirrorImg() functions that already exist in SpatialExperiment as you also need to update the spatialCoords() for cases like the one @Nick-Eagles ran into.

Best, Leo

[drighelli]

Hi guys (@lcolladotor @Nick-Eagles),

thanks for standing up for this PR, I think that this could be a useful functionality for the class for cases like the one mentioned by @Nick-Eagles!

If you make such a PR we'll be happy to revise the code and merge it into the main branch once it all looks ok.

Thanks, Dario

[lcolladotor]

Hi Dario!

Thanks, this is great. This will be @Nick-Eagles's first PR, so it'll be a good training opportunity for him. We met earlier and explored a bit the internal code of SpatialExperiment. Overall, I think that the PR might go smoother if you can help us clarify a few things:

• Should Nick's function return an SPE object? We saw that for example rotateImg() and mirrorImg() return an SPE. We were debating whether to return an SPE or to return the input for spatialCoords().
• We saw that rotateImg() and mirrorImg() take a sample_id argument. So well, if you had to update the spatial coordinates for a few samples, users would need to run sequential runs of Nick's function (as well as these other ones). We'll follow the current design of taking a sample_id argument, subsetting, updating the internals for the given sample, then returning the SPE, but well, we wanted to verify that this was ok.
• Where should we include examples for this new function? One option was to do so on the same documentation file as rotateImg() and mirrorImg(). Though well, Nick's function doesn't actually edit the imgData(). Another option was to create a new documentation file and also run rotateImg() and mirrorImg() on the example code, since that's what most users would want to do.
• Since well, users might want to rotate and image and the spatial coordinates together, should we even plan to include a wrapper for both? Or maybe leave that for a 2nd PR?
• Should the methods code be included in R/imgData-methods.R, R/SpatialExperiment-methods.R or maybe a new R/spatialCoords-methods.R? The spatialCoords() methods are in R/SpatialExperiment-methods.R and are documented along the SPE doc. I'm leaning towards either imgData-methods.R (specially if it's documented in that same Rd file) or a new spatialCoords-methods.R (if it's documented separately).

Or would it be easier if we had a 30 min Zoom call before Nick sends the PR?

Best, Leo

[HelenaLC]

Couple thoughts here-

• Not sure if you saw, but there already are the .rotate & .mirror functions here. So please make use of these in order to avoid redundancies & facilitate unit-testing. Will have to get rid of the as.raster() at the end, but other-wise, these will work for both images & coordinates.
• rotateImg() and mirrorImg() return whatever the input is. So if it's a SPE, a SPE. But if it's a SpatialImage, they return that. I'd personally prefer sticking to a in = out implementation as I find it most intuitive. To make things general, we could implement methods for both SPE and matrix, i.e., the latter would return the input for spatialCoords() as you mentioned.
• I'd not put this in the rotateImg() and mirrorImg() docs, as it's unrelated. See next point.
• Regarding the last 2 points: I think the easiest and most comprehensive would be to have separate functions for image & coordinate transformations (i.e., different names), plus a wrapper for both (for user-friendliness). Then document them separately in the imageData-methods and SpatialExperiment-methods. And the wrapper in the latter as well. Something like rotate/mirrorImg, rotate/mirrorCoords and rotate/mirror???- I don't know...

Alternative: While writing this, a perhaps "cleaner" way (but I'm not sure), would be to have only rotate/mirrorData with an argument which = c("spatialImage", "spatialCoords", "both") or something - which can do everything. Of course, if the signature is a spatialImage, it defaults to that...

Just my thoughts, happy to have a quick zoom sometime... I definitely agree we should discuss before doing a PR to avoid repeated time spent on both sides 🙏

[Nick-Eagles]

Sorry for the delay-- could we meet (zoom) next week as some point? I've created a when2meet and tried to span times appropriate for everyone's time zones: https://www.when2meet.com/?14874569-cKL3E. Thanks!

Also, to quickly respond to your ideas (though I'm sure we can discuss more on zoom):

• I did see the .rotate and .mirror functions and agree with the value of using them wherever possible, rather than writing multiple instances of similar code. However, I felt that it was a bit unnatural to try to apply these functions to spatialCoords, which are a matrix of values rather than a raster whose indices represent the same data as those values (hope that makes sense). I'm sure we can discuss this more over zoom.
• I personally like the idea of a rotate/mirrorData with argument which = c("spatialImage", "spatialCoords", "both"), but obviously I can base my implementation around your choice.

[lcolladotor]

Thanks Nick @Nick-Eagles! I just filled out the when2meet info. Helena @HelenaLC, if next week doesn't work we can expand the when2meet to other weeks. Or if you have a preferred time, we can try to switch meetings on our end to make it work.

Dario @drighelli and Lukas @lmweber, feel free to join us, though from talking to Lukas our understanding is that Helena would likely review this PR by Nick.

Best, Leo

[lmweber]

Happy to join the meeting too. I have filled out the when2meet.

[HelenaLC]

Filled out, free any time in the afternoons!

[drighelli]

I cannot next week, but feel free to meet without me :)

Thanks!

[lcolladotor]

Thanks for responding! I sent a calendar invitation and we'll keep you updated Dario.

Best, Leo

[drighelli]

Thanks!

[lcolladotor]

Hi everyone,

Here's a summary from our meeting today.

• Nick has a working function trans_geom() at https://github.com/LieberInstitute/spatial_ReferenceMapping/blob/main/code/01-apply_transformation/01-geometric_transformation.R (private for now, but we put the code on this gist).
  • Note: we agreed that the x and y columns should not be harcoded like they are at these lines
• We'll try to use .rotate() and .mirror() to reproduce what trans_geom() can currently do. Basically, following a test-driven framework.
• In our branch, we agreed that we could move the as.raster() calls in .rotate() and .mirror() to outside these functions https://github.com/drighelli/SpatialExperiment/blob/25c25cd9747279c5dd293b2aa5e12398e92bf851/R/SpatialImage-utils.R#L65.
  • We'll need to make sure that we use as.raster(.rotate()) where .rotate() is currently used (same for .mirror()).
• We'll try implementing Helena's idea based on indexes:
  • convert the 2-column x, y data.frame to a matrix of indexes
  • transform that matrix using .rotate() and .mirror() if possible
  • convert back to a 2-column x and y data.frame

• We'll add our code at SpatialExperiment-methods.R
  • Code for can both rotate/mirrorCoords
  • Code for a function that can do that + the same for the img. That is the rotate/mirror functions
  • So no function that can both rotate and mirror at the same time.
  • Document examples on SpatialExperiment-methods.R
• Add unit tests at test_SpatialExperiment-methods.R. If we use .rotate() and .mirror() then we won't have to write unit tests for that internal code.
  • We'll also test internally ourselves against trans_geom() although those tests won't be a part of the PR per-se. We'll show a reprex on the comments of the PR.
• If we can't use .rotate() or .mirror(), that's ok. Once we send the PR, then others can look at the code and it'll make it easier to ask questions and to continue improving the PR. For example, we could chat on the SpatialExperiment Bioconductor Slack channel.
• Lukas will send us his .Rproj file so we can try to use the same coding style as much as possible. We'll also try not avoid editing other files (like line endings etc).

[lmweber]

Here are my .Rproj settings:

Version: 1.0

RestoreWorkspace: Default
SaveWorkspace: Default
AlwaysSaveHistory: Default

EnableCodeIndexing: Yes
UseSpacesForTab: Yes
NumSpacesForTab: 2
Encoding: UTF-8

RnwWeave: Sweave
LaTeX: pdfLaTeX

BuildType: Package
PackageUseDevtools: Yes
PackageInstallArgs: --no-multiarch --with-keep.source

[lcolladotor]

Thanks Lukas!

Shouldn't NumSpacesForTab: 2 be NumSpacesForTab: 4 instead?

Here's the one I use in spatialLIBD for reference:

% more spatialLIBD.Rproj
Version: 1.0

RestoreWorkspace: Default
SaveWorkspace: Default
AlwaysSaveHistory: Default

EnableCodeIndexing: Yes
UseSpacesForTab: Yes
NumSpacesForTab: 4
Encoding: UTF-8

RnwWeave: knitr
LaTeX: pdfLaTeX

AutoAppendNewline: Yes
StripTrailingWhitespace: Yes
LineEndingConversion: Posix

BuildType: Package
PackageUseDevtools: Yes
PackageInstallArgs: --no-multiarch --with-keep.source

[lmweber]

Ah yes, in this project we have been using 4 spaces for tabs.

I usually use 2, so here I have always been hitting tab twice. So yes, if you put 4 in your .Rproj it should be consistent automatically. Thanks!

[lcolladotor]

@Nick-Eagles was this issue resolved by #105? Or is there anything pending?

Thanks!

[Nick-Eagles]

This was indeed completed by #105, so I'll close this issue.