#spatialexperiment-devel

2021-03-19

Lukas Weber (14:00:00): > @Lukas Weber has joined the channel

Lukas Weber (14:00:00): > set the channel description: Private channel for discussions by SpatialExperiment developers / maintainers

Stephanie Hicks (14:56:25): > @Stephanie Hicks has joined the channel

2021-03-20

watanabe_st (01:58:35): > @watanabe_st has joined the channel

watanabe_st (01:59:11): > @watanabe_st has left the channel

2021-03-23

Helena L. Crowell (08:15:10): > @Helena L. Crowell has joined the channel

Dario Righelli (11:45:27): > @Dario Righelli has joined the channel

Davide Risso (11:45:28): > @Davide Risso has joined the channel

Lukas Weber (11:46:12): > set the channel topic: Private channel for development discussions by core developers of SpatialExperiment package

Lukas Weber (11:46:35): > set the channel description: Private channel for development discussions by core developers of SpatialExperiment package

Lukas Weber (11:58:26): > Hi all - some points to get the discussion started in this channel. We can use this channel for our internal discussions by the core development team of theSpatialExperimentpackage, and the other channel#spatialexperimentfor discussions with the broader Bioc community. > > Some suggestions for priorities over the next few weeks (and feel free to add additional comments below): > * An initial update with various small changes for consistency with Bioconductor code style and formatting (assigned to@Helena L. Crowell). This will affect quite a few files, each in small but related ways, so my suggestion would be to do this as one final “big pull request”. Then this gives us a good base for making all future updates as small individual pull requests that are easier to review. > * We work together on a set of development guidelines that we can add to the GitHub readme. I’ll start a Google Doc and post the edit link in our private chat so we can all add to it. > * Add comprehensive unit tests - maybe@Dario Righelliand@Helena L. Crowelleach focus on tests for various parts > * Update documentation and help files (e.g. make sure all functions, arguments, inputs, outputs, etc are clearly documented) - I would be happy to volunteer for this > * Update vignette and examples in help files (after all above is done, to avoid merge conflicts) > * If we think of any additional class design issues, then open these as GitHub issues for now, so we can revisit them in a few weeks after above is done. > Let me know what you think, and feel free to add further points below. I think we should be able to make some good progress on this now over the next few weeks.

2021-03-24

Stephanie Hicks (11:43:20): > This sounds reasonable to me. My only question is what this “initial update” will look like. We might discuss this all together at our next meeting so we get input from everyone (as opposed to just assigning it to@Helena L. Crowell)?

Helena L. Crowell (11:51:24): > Yes, that might have sounded a bit “scary” above:see_no_evil:Basically what I had in mind was a single PR dedicatedsolelyto coding style, with thepromisethat I won’t touch anything else. This means tab-indentation, spacing around logicals and between commas, no spacing after argument =s, no @ accessions, trying to keep the 80-char limit etc. etc. > > So really, only style-related things, which shouldn’t take more than 1-2h, and all else will be left to open discussion & small PRs. But that was just an idea I thought would be useful to “clean up” before we really get started:slightly_smiling_face:!! Also, even for this, we canof coursediscuss / change things afterwards. But it would be neat to at least i) eradicate the obvious style-breaks and ii) make things consistent. > > But yes, we can certainly discuss this in the next meeting! But I think the Bioc code guide is already a good place to start:slightly_smiling_face:

2021-03-25

Helena L. Crowell (07:42:18): > @Dario Righelli@Lukas WeberI guess the question is, should I i) do a code-style PR over the next couple days so we can discuss it Tue or ii) wait until we discuss? I vote for i) simply because I feel it might be easier to see and discuss than the other way around. But happy to wait as well if you prefer. Also, for this one, I think all 3 of us should accept the PR before it’s merged so that we’re all content with the style. My target is to open up SCE for inspiration, and the Bioc guidelines of course, and pass BiocCheck with as few notes as possible.

Lukas Weber (09:02:42): > Yes, I think that is a good plan, thanks@Helena L. Crowell- so you prepare this PR over the next couple days (which passes BiocCheck with as few notes etc as possible), and then both@Dario Righelliand I can aim to review it before our meeting, and we use this to continue our discussions in the first meeting

Helena L. Crowell (09:03:41): > What do you think@Dario Righelli?

Dario Righelli (09:14:51): > Hi sorry for the late reply, it was a busy morning, I agree for pushing ahead with the code restyling. > I take this opportunity to tell that I already tried to fix the GHA, configuring them with the missing needed packages. Now only the checks fail, but I’m not able to understand if they are due to some other missing packages or to some errors in the code/tests. > Please let me know if you have any clue about this so we can discuss about it on Tuesday:slightly_smiling_face:

Lukas Weber (09:56:57): > Oh cool, yes good idea to fix the GitHub Actions too. I can try to have a look at that too

Dario Righelli (10:33:20): > thanks!

Helena L. Crowell (11:10:50): > Okay, I’m about done. However, there’s one point where I’d be happy for some feedback: > > We currently have a mixture of > > ... <- function(...) > { > ... > } > > ... <- function(...) { > ... > } > > if (...) > { > ... > } else { > ... > } > > if (...) { > ... > } else { > ... > } > > Now, I think we need to make a decision here for both consistency and readability. Having the line-break makes the code too lose and hard to read in many cases, and nearly doubles the number of lines in e.g. nested if-else statements. So I think having it everywhere is not an option. I see two options: > 1. Remove it everywhere and always have... {in one line, possibly adding a blank line afterwards to separate function definition and content. > 2. Remove it within functions (includinglapply, if-else-statements etc.), but keep it for function definitionsonly. > 3. ??? > Thoughts?

Lukas Weber (11:49:26): > I usually put the squiggle bracket on the same line, so... {- so this would be my vote for everywhere. But I think the most important thing is consistency within the package, so it is easy to read.

Dario Righelli (12:07:18): > I usually use the line break because I find it more readable, but I understand that it’s not so common, so as you prefer.

2021-03-26

Helena L. Crowell (06:05:25): > Okay, I did the PR from branchcode_style. All is up for discussion on Tue of course. Also, open 3 (?) issues to ponder about… Have a good weekend!

Dario Righelli (06:09:52): > Thanks Helena, could you please do me a favour and check if the “merge” button appears in the PR? > In my experience It should, as you and Lukas are already collaborators on the repo, but otherwise we have to check if there are some other options to abilitate. > Thanks again.

Helena L. Crowell (06:14:08): > Yes, it does, but I think it shouldn’t in this way - e.g. for our lab website, the merge button is faded out and can only be clicked if someone reviewed the PR. Currently, it looks like I can just merge it myself: - File (PNG): image.png

Dario Righelli (06:15:46): > the important thing is that it appears for everyone:wink:

Dario Righelli (06:16:18): > Of course in our guidelines we can specify that no one can self merge PRs

Dario Righelli (06:17:00): > I’m learning more about github during this project than ever:sweat_smile::joy:

Helena L. Crowell (06:17:02): > Yes, of course, but I think there’s an option to set it so that someone has to review first. This is also useful because you get a notification ‘X accepted your PR’ so that you know it’s ready to merge

Dario Righelli (06:17:32): > I never saw it, can you please show me where is it?

Helena L. Crowell (06:17:42): > One sec, I’ll try to find it

Helena L. Crowell (06:19:31): > There’s instructions here how to “protect” a branch (in our casemaster), so that merging is only possible after a minimum of X reviews:https://docs.github.com/en/github/administering-a-repository/managing-a-branch-protection-rule

Dario Righelli (06:20:28): > ok thanks, maybe we can put this point in the agenda so we can all discuss about it, what do you think?

Helena L. Crowell (06:23:18): > I thought we already discussed this: That at least one other person has to approve before merging, no?

Dario Righelli (06:28:33): > that’s for sure! I was only talking about the github setup

Lukas Weber (09:13:04): > Thanks@Helena L. Crowell! I’ll have a look at this branch > > Yes@Dario Righellithere is an option somewhere in GitHub to “protect” the master branch so nobody can merge without review (although technically it still allows self-review), which we could try activating. I can add a point on this in the guidelines too

2021-03-30

Helena L. Crowell (09:37:31): > I left comments on all 3 issues summarising what we discussed (correct me if I got it wrong or missed something!). Maybe once the code-style PR goes through,@Dario Righelliand I can discuss how to best split these. And@Lukas Webercan get going on documentation whenever he likes:slightly_smiling_face:

Lukas Weber (10:22:47): > This sounds like a good plan, thanks!

Dario Righelli (12:06:42): > Hey guys, went through the guidelines, everything seems fine to me. Just added a few comments to solve some doubts:slightly_smiling_face:

Lukas Weber (12:08:47): > Great, thanks!

Dario Righelli (12:13:04): > thanks Lukas for doing this:slightly_smiling_face:

Dario Righelli (13:19:21): > Hi guys, I’m looking into the github branch rule(s), but there lots of them, can you take a look to the point 6.1 and 7.1, 7,2 ?

Dario Righelli (13:19:36): > Do you think we need them too?

Dario Righelli (13:19:43) (in thread): > https://docs.github.com/en/github/administering-a-repository/managing-a-branch-protection-rule

Lukas Weber (13:39:17): > Oh, hmm, will have a look. Maybe there are more settings to choose from than I realized

Lukas Weber (13:46:40): > Ah ok, yes there are a lot of options. I would suggest minimal additional options, so we still have flexibility to e.g. push something if we know the check from Windows is broken or similar and we have discussed this. So from my look at it now, I think the settings we need are: (i) Branch name “master”, (ii) “Require pull request reviews before merging”, (iii) “Include administrators”, (iv) “Allow force pushes”, (v) “Allow deletions”; and then from the dropdown select that at least 1 approved review is required. Maybe there are additional settings I missed though.

Lukas Weber (13:47:59): > And for possible occasional “major” PRs we can discuss here if we want at least 2 reviews for that one (e.g. the current one) - but I think we can do these ones with an honor system, since I don’t think it will be often

Dario Righelli (14:12:00): > ok thanks, let’s see what Helena thinks about this and then I’ll setup everything

Helena L. Crowell (14:51:10): > Agree with above - minimal restriction. As long as we have 1 required approval for a PR to go to master, the rest is all fine with me :)

Helena L. Crowell (14:51:56): > And agree, for major ones (eg the code style one), getting confirmation from all thru slack is nice

Helena L. Crowell (14:54:33): > In any case, I think we can adjust settings later on should we notice anything weird or annoying. For now just having the master protected in some way is all we need. I also don’t know about all the options, so let’s just see how it works :)

Lukas Weber (16:28:41): > I have started the submission process forSTexampleDataExperimentHub package now too. It is exactly the 2000th submission in the issue tracker:tada:https://github.com/Bioconductor/Contributions/issues/2000 - Attachment: #2000 STexampleData > Update the following URL to point to the GitHub repository of
> the package you wish to submit to Bioconductor > > • Repository: https://github.com/lmweber/STexampleData > > Confirm the following by editing each check box to ‘[x]’ > > I understand that by submitting my package to Bioconductor,
> the package source and all review commentary are visible to the
> general public. > I have read the Bioconductor Package Submission
> instructions. My package is consistent with the Bioconductor
> Package Guidelines. > I understand that a minimum requirement for package acceptance
> is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS.
> Passing these checks does not result in automatic acceptance. The
> package will then undergo a formal review and recommendations for
> acceptance regarding other Bioconductor standards will be addressed. > My package addresses statistical or bioinformatic issues related
> to the analysis and comprehension of high throughput genomic data. > I am committed to the long-term maintenance of my package. This
> includes monitoring the support site for issues that users may
> have, subscribing to the bioc-devel mailing list to stay aware
> of developments in the Bioconductor community, responding promptly
> to requests for updates from the Core team in response to changes in
> R or underlying software. > I am familiar with the Bioconductor code of conduct and
> agree to abide by it. > > I am familiar with the essential aspects of Bioconductor software
> management, including: > > ☑︎ The ‘devel’ branch for new packages and features. > ☑︎ The stable ‘release’ branch, made available every six
> months, for bug fixes. > ☑︎ Bioconductor version control using Git
> (optionally via GitHub). > > For help with submitting your package, please subscribe and post questions
> to the bioc-devel mailing list.

Lukas Weber (16:30:19): > (I will update the objects with the final SPE structure again before the release date, after we finish the current round of updates on the class design.)

2021-03-31

Helena L. Crowell (02:02:18): > Are we still waiting with the PR for something? Eg Shall I fix the version to be in line with guidelines & update NEWS? So we can get started on the rest

Helena L. Crowell (02:03:09): > Lukas approved already. So maybe once I do this fix, Dario can approve too? Or let me kno if there’s anything else I should fix

Dario Righelli (05:12:32): > Sorry I just started slack… I’m going through your PR now:sweat_smile:

Dario Righelli (05:12:44): > I’ve also created the rule on the master branch

Dario Righelli (05:17:07): > Approved

Dario Righelli (05:17:29): > Thanks for the re-styling

Helena L. Crowell (05:18:07): > Cool, thanks! So I will merge it now, okay? And then after I get a coffee maybe we can decide how to split the issues?

Helena L. Crowell (05:42:00): > Okay, so, what’s your preference for the issues? Maybe you can cover moving the spatial coords to int_colData and I cover the spatialData? I’m not sure what#s the best way, because we might work on the same scripts to do these things as they are connected

Helena L. Crowell (05:43:38): > Also, the sample ID issue - so updating the colData replacement and cbind example. One option would even to be make sample_ids unique incide cbind with a warning, if they are not unique beforehand. Then the user doesn’t have to do this to get it to work

Dario Righelli (05:46:44): > ok for me to work on the spatialcoords. Do we want to create a first branch from the master and the two branches from that?

Helena L. Crowell (05:47:39): > yes, I think that’d be good - let’s make a branchspatialCoordsDataand you and I make a branchspatialCoords``spatialData?

Helena L. Crowell (05:48:32): > Also, I think we can both just PR tospatialCoordsDataand look at each others changes, then run tests on the merged branch to see everything works

Dario Righelli (05:49:36): > just to recap, I move the spatialCoordsNames into the int_metadata and split the coordinates (taken as input arguments from the constructor) into the int_coldata > It more of a question than a recap:smile:

Helena L. Crowell (05:51:21): > 1. we don’t need to track the spatialCoordsNames anymore. The accessor / replacement should simply return / replace names(int_colData()\(spatialCoords) (this is also what reducedDimNames does btw) > 2. yes, the constructor should put the spatialCoords as a column in int_colData > 3. also, I think the validity needs some updating to assure i) int_colData()\)spatialCoords is a numeric matrix

Dario Righelli (05:53:16): > Sorry, the spatialCoordsNames becomes spatialDataNames is that right?

Helena L. Crowell (05:54:10): > almost. spatialDataNames is a subset of names of colData. so it does not contain spatialCoordsNames. However, I think we agreed that spatialData has an option to include both colData and/or spatialCoords as well

Helena L. Crowell (05:54:45): > e.g. spatialCoordsNames: x, y, z > spatialDataNames: in_tissue, array_col, array_row

Helena L. Crowell (05:55:27): > I think something like this would be good: > spatialData <- function(spe, spatialCoords = TRUE/FALSE, colData = TRUE/FALSE/character vector)

Helena L. Crowell (05:56:18): > and, we fix spatialData to DFrame and spatialCoords to matrix (analogous to colData and reducedDims). No other options to avoid going down a rabbit whole:stuck_out_tongue:

Dario Righelli (05:56:25): > I was imagining that the user provides the spatialData to the constructor specifying the spatialCoords names (i.e. c(“x”, “y”)) and that we split the coordinates from the rest of the spatialData. > The coordinates go into the int_colData and the other columns into the colData

Dario Righelli (05:57:06): > I think we are saying the same thing…:smile:

Helena L. Crowell (05:57:08): > Yeah, that makes sense. Or, if spatialCoords is a matrix (not character vector), you can use that

Helena L. Crowell (05:57:27): > so spatialData = … spatial Coords = c(“x”, “y”) or matrix will both work

Helena L. Crowell (05:58:02): > Okay, how about we make a branch and we can discuss if any other issues arise?

Dario Righelli (05:59:04): > sure!

Dario Righelli (05:59:11): > do you want to start the branches?

Dario Righelli (06:00:00): > If you prefer I can do it

Helena L. Crowell (06:00:07): > I dont mind, you do it:stuck_out_tongue:

Helena L. Crowell (06:00:48): > Also: I think it would be good if we both write unit tests immediately for this so we dont intoduce errors and check that everything works or at least doesnt have obvious bugs

Dario Righelli (06:01:56): > oh I’ve a question, have you also solved the BiocCheck error related to the git pack file?

Helena L. Crowell (06:02:35): > uh, no idea… do you mean this big git related file? I thought lukas said he’d look into this? I dont know how to solve it:see_no_evil:

Dario Righelli (06:13:01): > Me neither… I tried with the help of the git guys and some guides on the Internet, but all the files stored in that pack are files that we’re using, so I think that’s the lowest number of Mb that we can reach…

Dario Righelli (06:15:45): > Anyway, now you should be able to see the new branches on the github. Of course they are all equal to the master at the moment

Dario Righelli (06:16:49): > So you work on thespatialDataand I work on thespatialCoords, then we both PR on thespatialCoordsDataand pray a God that everything works as we planned:joy::grimacing:

Helena L. Crowell (06:17:48): > Great, thanks!! Hahaha, yeah - otherwise I’m sure we’ll figure it out somehow:stuck_out_tongue:

Helena L. Crowell (06:19:15): > One minor thing, I think we both want to update this … to this: > > setClass("SpatialExperiment", > contains="SingleCellExperiment", > slots=c( > spatialData="DataFrame", > spatialCoordsNames="character")) > > setClass("SpatialExperiment", > contains="SingleCellExperiment") >

Dario Righelli (06:54:46): > I can work on this starting from tomorrow

Helena L. Crowell (07:09:50): > ugh… this is really difficult… I need the spatialCoords to do the spatialData updates…

Dario Righelli (07:18:12): > mmm I’ll try in the afternoon but I’m not sure I’ll be able to do it

Helena L. Crowell (07:20:06): > Would it be okay if I do some changes at least to get the example running? I currently cant construct an SPE to write tests or see if the methods even work:confused:I’m really sorry… maybe this issue is just too connected…

Helena L. Crowell (07:20:58): > I can wait if you prefer. Not sure if changing the spatialCoords will be any easier without touching the spatialData

Dario Righelli (07:24:44): > nono sure go ahead! I’m so sorry for this…

Dario Righelli (07:25:07): > It’s just that this weeks I have too much things all at the same time

Helena L. Crowell (07:26:14): > No, don’t worry - I didn’t realize how connected the issues are. I think in the future we’ll have plenty of stand-alone fixes. but this one is just difficult to split I just realized:confused:Happy to fix it! I think things are getting much simpler anyways, so the changes are easy mostly:slightly_smiling_face:

Dario Righelli (08:21:43): > thanks!

Dario Righelli (09:11:58): > Anyway,@Helena L. CrowellI think I can help today if you need something

Helena L. Crowell (09:15:34): > trying something out right now… do you think it would make sense to have all spatialData, spatialDataNames, spatialCoords, spatialCoordsNames in the construct? E.g., in principle, if xNames are supplied, these could be extracted from the input SCE’s colData

Dario Righelli (09:18:05): > mmmspatialDataNamesare something likecolnames(spatialData)[-spatialCoordsNames]

Dario Righelli (09:18:17): > as you were saying yesterday

Dario Righelli (09:18:53): > maybe just having thespatialDataand theSpatialCoordsNamesshould be enough at this point, this should avoid misunderstanding to the final user.

Dario Righelli (09:19:25): > also because thespatialDatacould be just aDataFramewith thespatialCoords

Dario Righelli (09:19:48): > what do you think?

Dario Righelli (09:20:01): > oh sorry now I understand what you mean

Dario Righelli (09:20:55): > because if it was already anSCEwe can just pass thespatialDataNamesand extract them from thecolData

Dario Righelli (09:22:44): > so maybe only thespatialData,spatialDataNamesandspatialCoordsNames? We can always retrieve thespatialCoordsfrom thespatialCoordsNames

Dario Righelli (09:23:21): > mmm

Helena L. Crowell (09:23:52): > yeah I was thinking… in principle you could make a SPE from a SCE without any additional data frames by just specifying columns in colData. In a way that makes a lot of sense I think, we just need to explain it well and think about what we prefer: So if both are specified, which do we take. Other then that, I think this would make things a lot more flexible

Dario Righelli (09:24:36): > yes for sure this is a case scenario we didn’t think about yesterday

Dario Righelli (09:26:41): > Maybe, I think that if both are specified we can > 1. stop the execution > 2. choose the spatialData over the colData[,spatialDataNames] producing a warning

Dario Righelli (09:27:00): > these are exclusive possibilities (logic OR)

Helena L. Crowell (09:27:46): > I think intuitively, > > I would prefer spatialCoordsNames & spatialDataNames get priority. > Whereas spatialCoordsNames can be in spatialData or colData, and spatialDataNames should be in colData. > If names are missing, we require spatialCoords/Data to be specified as a separate matrix/DFrame. > > what do you think?

Dario Righelli (09:28:26): > mmm this makes sense, but what happens if the spatialData are provided at the same time?

Helena L. Crowell (09:28:27): > Because this would be “easiest” in terms of getting a SCE to a SPE, by just setting up a big colData frame that contains everything

Helena L. Crowell (09:29:08): > I’d just ignore it… and give a message that “we notice both are specified, but use the spatialDataNames”

Dario Righelli (09:29:28): > makes sense

Helena L. Crowell (09:29:47): > I’ll try doing this and see how it goes:see_no_evil:I didnt think of all the options yesterday to be honest…

Dario Righelli (09:30:33): > yeah neither all of us I suppose:sweat_smile:

Helena L. Crowell (09:30:47): > But apart from the constructor, the good news is: I feel like everything is really much easier so far! Both to understand for the user, and for us to maintain.

Dario Righelli (09:30:58): > I’m sure lots of case scenarios will come up once the people start to use the class

Dario Righelli (09:31:43): > anyway, the spatialCoords aren’t necessary anymore in the constructor right?

Dario Righelli (09:32:04): > we expect them into the spatialData or in the colData

Helena L. Crowell (09:32:06): > I think we can keep it for completeness. So if spatialCoordsNames aren’t provided, this can be a matrix

Dario Righelli (09:32:43): > mmm yes, but I’m afraid that it could be misinterpreted from the users

Helena L. Crowell (09:32:50): > so you could do > SPE(spatialCoords = matrix, spatialData = DFrame) > or SPE(spatialCoordsNames = x, y, z, spatialDataNames = a, b, c) > or combinations of that

Dario Righelli (09:33:25): > there could be the case where the spatialCoords are used and all the others are not…

Helena L. Crowell (09:33:35): > Hm I don’t think so. spatialCOORDS is a matrix, spatialCoordsNAMES a character. > We give preference to NAMES to extract from the colData, with a message if both are supplied.

Helena L. Crowell (09:33:58): > Yes, so you could do SPE(…, colData = NULL, spatialCoords = matrix) and this will work

Dario Righelli (09:34:02): > mmm okok

Dario Righelli (09:34:14): > we have to intercept any case I suppose

Dario Righelli (09:34:30): > to provide informative messages

Helena L. Crowell (09:34:37): > I think I’ll just start writing some test for all combinations and see if I can get this to work for all scenarios

Dario Righelli (09:34:53): > okok let me know if I can help

Helena L. Crowell (09:34:58): > We can always simplify it if it really is getting weird

Helena L. Crowell (09:35:44): > Thanks! And thanks for the discussion. It helps a lot to think this through when I didn’t think about it earlier and suddenly I got confused:stuck_out_tongue:

Dario Righelli (09:36:18): > oh no worries, anytime!:smile:

Lukas Weber (10:10:08): > Ok cool, yes sounds like these issues are quite inter-connected > > I also saw your comments about about the large files in git history. I remember looking into this - it should be feasible with git repo-cleaner together with contacting the Bioc team. I would be happy to try this. However I wasn’t sure if it needs to overwrite the old git commit hashes in the history, so probably safest if I wait until the above changes are all done, so we don’t end up with unmergeable branches

Helena L. Crowell (10:48:16): > quick Q- for read10x, the coords are currently array_row/col. but shouldnt these be pxl_row/col_in_fullres? they’re relatively the same, but this probably really matters for when we apply scale factors to match the image

Lukas Weber (11:01:43): > I think pxl_row/col is better, since that has consistent scaling for both x and y (in terms of pixels) - I believe array_row/col has different scaling for x and y because the grid is hexagonal (not square)

Lukas Weber (11:02:24): > (i.e. there are 3 spots per hexagon in y direction, and 4 spots per hexagon in x direction - or the other way around)

Lukas Weber (11:02:51): > so using array_row/col slightly squishes the image in one axis

Helena L. Crowell (11:09:49): > Also, would you both agree we consistently use SPE for examples, documentation, etc. and possibly internally also? currently, I used ve in read10xVisium, and we have se somewhere (which is already taken by SummarizedExperiment). So my proposal would be to replace both with spe - what do you think?

Lukas Weber (11:12:51): > Ah, you meanspefor object names? I think I have mostly been using this. So yes, that is fine for me to make sure I consistently use it

Helena L. Crowell (11:26:04): > is the seqFish vignette used at all? Running checks now and it’s failing… so not sure if I want to fix that or we anyways needs to re-write the whole thing with proper data at one point? It looks quite old to me… e.g. there’s aggplot()line but this is not in our DESCRIPTION. I’m actually super confused why this didn’t come up before:open_mouth:

Dario Righelli (11:42:51): > the ggplot section has eval=FALSE

Dario Righelli (11:43:12): > I think

Helena L. Crowell (11:43:15): > aha! sorry, my bad:see_no_evil:okay, then I just need to adjust 1 argument and it works

Helena L. Crowell (11:43:40): > maybe I just drop the ggplot chunk then? since we dont want to add more to suggest, what do you think?

Dario Righelli (11:45:01): > it depends if we want to give a hint on how to plot seqfish data or not…

Dario Righelli (11:45:26): > that’s why I set eval=FALSE, to not add ggplot2 as suggest

Dario Righelli (11:45:39): > but in general I think we can drop the chunk

Dario Righelli (11:45:52): > thinking about it, we have also the ggspavis for visualization…

Dario Righelli (11:46:26): > sorry again, I was writing and thinking at the same time!:smile:

Helena L. Crowell (11:47:08): > Yeah, I know what you mean because I always like visualization… but then again its a class package, so maybe we can write a section in the vignette that points to other resources?

Helena L. Crowell (11:47:41): > I also don’t mind to much to keep the chunk unevalutated, but this gives a note in the BiocCheck because they recommend we don’t have unevaluated code:confused:

Dario Righelli (12:04:35): > you’re right!:+1:

Helena L. Crowell (13:04:27): > I think I fixed thecbindto 1) give a message when sample_id’s are duplicated, but make them unique and still cbind; and 2) cbind normally if they are all unique. BUT I don’t know what thisdeprase.level=1is - do you know? I’m still using it, but not sure what it does

Dario Righelli (13:05:38): > No I’m not, I had to put it there for consistency with thecbindmethod defined in theSummarizedExperimentclass:sweat_smile:

Dario Righelli (13:07:43): > Marcel pointed me this, because when using it withMultiAssayExperimentit failed somewhere…

Helena L. Crowell (13:17:48): > Okay haha - so I’ll just keep it!

Dario Righelli (15:31:19): > yes, I suppose that we have to…

2021-04-01

Helena L. Crowell (04:21:54): > Another design choice that we should discuss is the following: > Should replacing colData keep the spatialData protected? Eg colData<-NULL will drop everything right now, but I think it should keep the spatialData right? Meanwhile, spatialData<- should be used to remove or replace the colData columns specified by spatialDataNames. Thoughts?

Helena L. Crowell (04:23:05): > So basically it behaves like a separate slot, but we store it in colData to make it easily accessible

Dario Righelli (04:36:06): > it makes sense to me. In the same way, we drop only the spatialData when doing spatialData <- NULL without affecting the colData

Helena L. Crowell (04:37:36): > Ok. Should I try this and also the sample_id issue? Or I can leave it “broken” for now and we do this in another PR if you prefer. Once the PR is thru we can split the work to write unit tests and update the documentation

Dario Righelli (04:38:25): > I was wondering, is there any way to define a sort of show method for the colData ? > for example to show the colData without the spatialData data frame > also, could it be useful? This comes from the Lukas questioning about having an overwhelming colData DataFrame

Helena L. Crowell (04:39:59): > Yes, I thought the same! I already tried this but didnt finish yet :D - I’m on a train now for 3h, but will try to PR later today

Helena L. Crowell (04:40:48): > And as a heads up… many files changed ONLY because I renamed ve/se to spe… and of course moving the slots caused many changes. So please dont freak out. I will explain all in detail with the PR:pray:

Helena L. Crowell (04:41:16): > The core didnt change, but there were many minor changes when moving the slots…

Helena L. Crowell (04:41:53): > What’s left iscolData > Show > Validity > Unit tests > Documentation > (i think)

Dario Righelli (04:56:31): > About the sampleIDs as you prefer, but maybe could be better to split up the PRs. > It could be more modular and easy to fix possible bugs… What do you think?

Helena L. Crowell (05:05:37): > Sure- So should I still protect the spatialData or leave the colData completely

Dario Righelli (05:27:12): > right, maybe at the moment we can focus on leaving the things as they are and we can open a new Issue on this?

Dario Righelli (05:27:51): > otherwise you can be overwhelmed with all that stuff all together:sweat_smile:

Dario Righelli (05:28:01): > and we can give you some help on splitting duties

Helena L. Crowell (05:34:07): > Okay, I will do a PR then soon-ish. But I’d like to continue working on this - so maybe after the PR and open up some issues okay? Then we can split up the remaining issues. Meanwhile, I’d still like to continue writing more unit tests (independent of the remaining issues). Does that sound okay?

Dario Righelli (05:35:18): > sure, thanks again!

Helena L. Crowell (05:36:51): > I willl PR to spatialDataCoords (not master) until we fix the colData/sample_ids okay?

Dario Righelli (05:38:38): > yes, so we can pull from there

Helena L. Crowell (05:39:31): > okay, done. I also added the PR comments in the NEWS file

Dario Righelli (06:01:03): > Do you think that this error > > -- Failure (test_SpatialImage.R:53:5): SpatialImage in-memory caching works as expected -- > > names(cache$cached) not identical to paste0("file://", path). > > could be solved withfile.path(path)?

Helena L. Crowell (06:06:05): > Not sure because this doesnt look like a normal path… file path would just do / or  not ://

Dario Righelli (06:06:25): > Or by detecting the system info withif(Sys.info()$sysname == "windows")

Dario Righelli (06:07:08): > mmm depends, on linux you havefile://for direct accessing files

Dario Righelli (06:07:36): > I don’t know specifically the difference btw

Dario Righelli (06:08:12): > because I saw this mostly in web browsers

Dario Righelli (06:12:30): > anyway I’d change this line:expect_identical(names(cache$cached), c(LETTERS[1:3], paste0("file://", path)))into something os specific…

Dario Righelli (06:13:03): > it’s just a matter of how the systems handle the file paths

Dario Righelli (06:14:19): > something like > > if(is.windows()) { > expect_identical(names(cache$cached), c(LETTERS[1:3], path)) > } else { > expect_identical(names(cache$cached), c(LETTERS[1:3], paste0("file://", path))) > } >

Dario Righelli (06:17:38): > or “cleaner” > > if(is.windows()) { > patha <- path > } else { > patha <- paste0("file://", path))) > } > expect_identical(names(cache$cached), c(LETTERS[1:3], patha)) >

Dario Righelli (06:18:07): > as much as a patch could be clean:sweat_smile:

Davide Risso (06:19:19): > But why do you need the file:// on linux? Won’t patha work on unix?

Davide Risso (06:19:41): > I mean patha as defined on your second line above

Dario Righelli (06:20:29): > I guess the problem is how the cached file is returned fromnames(cache$cached)

Davide Risso (06:23:13): > Is this BiocFileCache? Perhaps you could ask Lori how this should be handled…

Dario Righelli (06:42:44): > I don’t know, I think so

Lukas Weber (20:57:47): > Hi all, I added a first draft of the core developer contributor guidelines as we discussed, modified from the notes in the Google Doc and added to the wiki:https://github.com/drighelli/SpatialExperiment/wiki@Dario Righelliand@Helena L. Crowellcould you please have a look at these and let me know if you have any additional suggestions? (Turns out the wiki is stored in a separate .git repo calledhttps://github.com/drighelli/SpatialExperiment.wiki.git, which is a set of .md files and with its own commit history etc. Thanks!)

2021-04-02

Dario Righelli (03:29:02): > Thanks a lot@Lukas Weber, it looks very good to me!

Helena L. Crowell (04:08:16): > Looks good to me as well! Thanks also for spelling this out so neatly & setting up the wiki

Helena L. Crowell (04:09:38): > @Dario RighelliNot sure if you’re working over Easter, but whenever you’re ready we could discuss what’s next / how to split remaining to-dos. I’ll write some issues for now and close the ones that should be reasolved

Dario Righelli (04:10:34): > I’ll come back on tuesday, just monday takeoff…

Dario Righelli (04:10:51): > I’m trying to better understand the test error now

Dario Righelli (04:11:13): > the problem is that I don’t have a windows machine…

Helena L. Crowell (04:12:43): > Yes, same for me. Did you see Aaron’s reply? What we could do is try his suggestion (which sounds pretty simple), do a PR to spatialDataCoords, and just see if it passes the check?

Dario Righelli (04:12:44): > May I try my idea on a new branch?

Dario Righelli (04:13:13): > Honestly, I don’t think it’s a matter of slashes.

Helena L. Crowell (04:13:15): > Yes, that’s a good idea! Just do a branch and PR - if it doesn’t work, you can make a change and PR again

Dario Righelli (04:13:40): > I’ll start from the spatialDataCoords branch

Dario Righelli (04:13:51): > I can use the spatialCoords branch maybe

Helena L. Crowell (04:13:51): > I really don’t know. Just do what you think might work & we’ll see:slightly_smiling_face:

Dario Righelli (04:14:04): > I want to try both, for completeness

Helena L. Crowell (04:14:17): > Up to you, or do a cacheFix branch or something, I don’t mind either way

Dario Righelli (04:14:31): > I wanted to use a docker, but there is no bioconductor docker based on windows

Helena L. Crowell (04:20:14): > Let’s try using the GH actions for now? I’ll work on some unit tests for now. When the cache issue is fixed, we can split the remaining issues (just opened one we discussed yesterday re colData replacement), ok?

Dario Righelli (04:27:41): > let’s waithttps://github.com/drighelli/SpatialExperiment/actions/runs/711134764:sweat_smile:

Dario Righelli (04:29:06): > I tried Aaron’s solution but it returns a weird warning about a not existing file …:man-shrugging:

Dario Righelli (05:01:09): > ok, it’d be solved thanks to Aaron of course:smile:

Helena L. Crowell (05:20:16): > Another question whie writing unit tests…. > > If we look at SCE for example, it fixes alt_exps and reduced dims in int_colData even if they are completely empty. > I did the same for spatialCoords and spatialDataNames > However, we currently allow imgData to be missing (int_metadata()$imgData = NULL). This is causing some issues /unexcepted behavior while writing unit tests. > > Would you agree to fix all of spatialCoords, spatialDataNames, imgData? Meaning: They have to exist, but can be empty. so replacing with <- NULL will clear them out and e.g. leave character() or an empty DFrame (that’s what SCE does for other slots as well) > > This makes it easier for us too, because we don’t need to check “is.null()” everytime, and can assume the slots exist

Helena L. Crowell (05:23:43): > (FYI I am writing unit-tests for the class validity, that’s why this came up…)

Dario Righelli (05:26:04): > yes

Dario Righelli (05:28:01): > Another question, do I have to squash and merge or just merge? I don’t know the detailed differences, github says that squash reduces the number of commits into the merged branch

Helena L. Crowell (05:29:30): > I think Lukas highly recommended to not squash, so just merge. It’s fine to have 100 commits, that’s still more traceable than having them “hidden” in one. In the end, we just want to keep a proper history of everything I think

Helena L. Crowell (05:29:54) (in thread): > Okay, so I will make a minor change to imgData so that it always exists

Dario Righelli (05:31:12): > okok, I think the squash came out from an Aaron suggestion last year and it remained as default preference in my browser. > I didn’t ever noticed it until now…

Dario Righelli (05:31:57): > I’m going to delete thecachefixand thespatialCoordsbranches

Helena L. Crowell (05:33:06) (in thread): > Yeah I deleted mine locally but do you know if I can do that also for the remote form the terminal? Without having to click on a button? > Ideally, after we merge we always delete the branch I think

Dario Righelli (05:34:45) (in thread): > git push origin –delete cacheFix

Dario Righelli (05:34:51) (in thread): > I usually do this

Dario Righelli (05:35:15) (in thread): > I also saw that there still is adevelbranch, do we want to remove it?

Helena L. Crowell (05:41:45): > Okay, so now: do you want to try the 2 colData issues? Meanwhile I’ll continue unit tests - we’re at 80% now:muscle:(but should probably cover lines more than once haha)

Helena L. Crowell (05:42:29) (in thread): > Yes, we should. Since we said to have designated branches for each issue vs. a general devel branch

Dario Righelli (05:45:33): > if you have time to do so it’s ok for me… I’ve a review of a paper and I’m already late:joy::sweat_smile:Otherwise, if you need some help, I can try to do both

Dario Righelli (05:47:22) (in thread): > ok I’m going to delete it

Helena L. Crowell (05:48:22): > I’m happy to do it because I’m bored at my parents’:smile:I’ll do a colData branch after wrapping up the validity unit tests

Dario Righelli (05:48:44) (in thread): > the repo sounds clean to me now, we just have a few branches related to the conferences

Helena L. Crowell (05:49:05) (in thread): > Yes, I think it looks really good now, too

Dario Righelli (05:49:49): > ok thanks a lot

Lukas Weber (08:29:18) (in thread): > thanks!

Lukas Weber (08:29:27) (in thread): > thanks!

Lukas Weber (08:38:37): > Ok this all sounds great. Following up a couple of points above: > * yes I think better to not do squash merge - then all the individual commits are shown in history, which makes it easier to go back and check/fix something if one of the commits had a problem. I think it is fine to have lots of small commits in history. > * for deleting branches, I usuallygit prunethem locally occasionally, and in GitHub I manually click on delete – but sounds like you can also push and delete > * I will start working on documentation PR, and also follow up with Bioc team later on the git large file sizes issue

Helena L. Crowell (08:42:15) (in thread): > Re doc - that’d be great & highly appreciated! I mostly left these untouched - so think we need a thorough one-by-one revision here to update arguments and description. > > Above all, it’d be good to have more thorough examples in both the dox and vignette, e.g. for in/valid replacements, possible use-cases we can think of etc. > > I am revising the colData replacement now and adding unit tests for cbinding (related) - these would be one place to start > > And of course, the largest change not in the vignette right now is that we moved slots… this will affect lots of documentation I suspect.

Helena L. Crowell (08:45:49) (in thread): > Oh and before I forgot… we currently have this free-floating seqFISH vignette… might be worth considering adding a proper vignette here for “moleculed-based” where we demonstrating getting in a bumpMatrix? Just an idea to be more general. The vignette is currently a little naked

Lukas Weber (08:46:46) (in thread): > Ah yes - I think that makes sense re vignettes

Dario Righelli (08:47:07) (in thread): > take a look at the rmd I sent in the private chat to lukas, I think it includes everything (too much stuff) but it shows everything until the latest working class version

Dario Righelli (08:47:45) (in thread): > But yes, we have to re-shape them I suppose

Dario Righelli (08:48:05) (in thread): > https://community-bioc.slack.com/archives/C01RSSNJ5PX/p1617208362067100

Lukas Weber (08:48:22) (in thread): > I have some example code using BumpyMatrix in the OSTA workflow chapter on seqFISH too - although that object is much larger, and will also need to re-shape the objects for the final structure

Lukas Weber (08:49:05) (in thread): > no wait - it is in themake-datascript inSTexampleData, and the object is then loaded in the OSTA workflow chapter

Lukas Weber (08:49:35) (in thread): > but yes will reshape those too

Helena L. Crowell (09:50:59): > Sorry,@Lukas Weber, another note re documentation: > Since this is the SPE package, the SPE constructor is probably important:wink:There’ve been some updates to this adding more flexibility (spatialData/Coords PR), and it might be neat having an example of creating the same object in different ways, e.g. one can > * make an SPE from a SCE using only colData, spatialDataNames, spatialCoordsNames > * …providing separate spatialData and spatialCoords > * …providing only spatialData and specifying spatialCoordsNames > * … > There are examples of all combinations in thetest_SpatialExperiment.Rscript! Of course, feedback on whether all this is intuitive and working well is welcomed. But while I was moving the slots etc., it seamed natural to support turning a SCE into an SPE without additional objects. For example, now one could just do SPE(assays, colData, rowData, metadata) and we create a valid SPE, where spatialData/CoordsNames can be specified afterwards via <- c(…)

Helena L. Crowell (13:50:02): > FYI I finished thecolDataissues a while back… but I’ll wait for the other PR to go through so we don’t get a change-overload:see_no_evil:But don’t feel rushed and enjoy the break - happy easter:slightly_smiling_face:

2021-04-05

Lukas Weber (21:16:30): > Hi all,@Dario Righelliand I are presenting on SpatialExperiment at the CZI Seed Network meeting on Wednesday. This is a meeting for various groups working on single-cell, spatial, multi-modal, etc data types, which are funded by the CZI. I have screenshotted a part of the program below so you can see some examples of topics. > > We have a 20 minute combined time slot for a presentation and demo.@Dario Righellipreviously sent a nice script with a demo and examples, so I was thinking we could split it up as (i) I do a ~7 minute presentation with some slides, (ii) followed by Dario with the demo, (iii) and then time for questions.@Dario Righellidid you have any additional ideas or preferences? > > The audience are people who are quite familiar with single-cell, spatial, multi-modal, etc - but who may or may not be familiar with Bioconductor or R (e.g. some of the groups are Python groups, or non-Bioconductor R groups). So I was thinking it would be good to start with some background on Bioconductor and why we think it is useful to have a core Bioconductor class on ST data, and then more details on the actual SPE class. This also gives an opportunity to mention our associated packages (STexampleData etc), as well as OSTA (which is now in a usable format, although still needs a lot of extending). > > My draft slides are below @Dario Righellicould you please let me know if this sounds like a good plan to you? And anyone else who has time, feedback on the slides also appreciated. Sorry it is a bit last-minute (presentation is on Wednesday). Thanks!

Lukas Weber (21:16:39): - File (PNG): CZI_program_screenshot.png

Lukas Weber (21:17:05): - File (PDF): CZI_SpatialExperiment_LWeber_DRighelli_HCrowell.pdf

2021-04-06

Dario Righelli (03:35:02) (in thread): > maybe you could mention the spot-based and the molecular-based families for spatial transcriptomics

Dario Righelli (03:35:06) (in thread): > not only the 10x visium

Dario Righelli (03:35:46) (in thread): > it works for me, but I think I have to shrink the demo, 7 minutes it’s not that much for it

Dario Righelli (04:00:59): > Hi@Helena L. Crowell, sorry to bother you, I saw that most of the “image accessors” are failing from the old version to the new one (imgPath ecc). > Could you please summarise the novelties ?

Dario Righelli (04:01:36): > I think we forgot to add them into the NEWS file

Helena L. Crowell (04:05:39): > Which version are you referring to? The former accessors (imgPath, imgUrl and imgGrob) don’t exist since Aaron’s restructuring of the SpatialImage class, and have been replaced by imgSource and imgRaster. These are unit-tested and functional. But this was the PR from March 2nd, unrelated to any resent changes. Also, since this is all on devel, I don’t think we necessarily want to assure backward compatibility, since this would require retaining deprecated slots and classes for a whole release cycle.

Dario Righelli (04:29:55): > I was referring to the Aaron’s PR + your adapting… Ok thanks I’ll go through the new vignette:smile:

Dario Righelli (04:32:11): > It’s ok to not ensure backward compatibility, I was not mentioning that. > I just want to adapt my demo vignette for the tomorrow presentation

Dario Righelli (04:34:41): > oh one last thing: what’ thedatacolumn into theimgDatastructure? I always forgot to ask

Dario Righelli (04:42:34): > @Lukas Webercheck this error, I think it’s because theSpatialExperimentdoesn’t export theimgGrobanymore. > maybe theimgRastershould be used instead (I guess) > > > library(ggspavis) > Error: package or namespace load failed for 'ggspavis': > object 'imgGrob' is not exported by 'namespace:SpatialExperiment' >

Helena L. Crowell (04:46:36) (in thread): > it contains theSpatialImages

Dario Righelli (05:27:06) (in thread): > ah ok thanks, because it looks always like this: > > DataFrame with 2 rows and 4 columns > sample_id image_id data scaleFactor > <character> <character> <list> <numeric> > 1 section1 lowres #### 0.0510334 > 2 section2 lowres #### 0.0510334 >

Dario Righelli (05:28:39) (in thread): > do you think that we can modify the showing of it in some way? to be more informative

Dario Righelli (05:29:14) (in thread): > or maybe changing thedatainto something likespi_data, just asking

Helena L. Crowell (05:30:42) (in thread): > don’t know how to / if we can change the show method:confused:I also don’t like it, but the issue is that this is the DFrame show method, and I don’t know how to change how it shows different classes… maybe someone else does?

Dario Righelli (05:32:30) (in thread): > me neither:joy:

Dario Righelli (06:05:38): > @Helena L. CrowellI went through the latest PR and approved it, thanks! :D

Dario Righelli (06:08:38): > I also got this error while doing this withTENxVisiumData > > spe <- TENxVisiumData("HumanHeart") > > spe > Error in `imgData<-`(`**tmp**`, value = `**vtmp**`) : > 'imgData' field in 'int_metadata' should have columns: 'sample_id', 'image_id', 'data', 'scaleFactor' > > I suppose you still have to update thespeobjects, maybe it’s preferable to wait until we have a final version (?)

Dario Righelli (06:09:47): > Because there are too many updated to do and I think it’s better to do them without any rush, I’ll stick with the old version of the vignette for the tomorrow presentation…

Dario Righelli (06:10:07): > Hope it works for anyone

Helena L. Crowell (07:05:12): > Yes, the 10x data is outdated… I agree that it’s best to wait with this. But I can easily update them after lunch if you like? Just that I won’t put this on Bioc just yet

Helena L. Crowell (07:05:38): > *it takes 5’ to update them, so I can do this if you prefer.

Dario Righelli (07:11:12): > no thanks, also the ggspavis and the STexampleData are outdated, it’s better to wait at this point. But thanks!

Helena L. Crowell (08:09:38): > @Dario Righelligreat, thanks for approving the PR! Beware, the next one is already on the way:see_no_evil:After this I think we’re good to go finishing documentation and unit tests. Some minor changes might be required in case I missed something, but this should be it regarding restructuring / the issues in GitHub at the moment.

Lukas Weber (08:42:35) (in thread): > good idea, will update

Lukas Weber (08:44:28) (in thread): > ah yes - ok will update to make it compatible

Dario Righelli (09:31:59) (in thread): > Thanks, no rush, I’ll stick the demo on the “old” working class :)

Lukas Weber (09:43:47) (in thread): > Ok, sounds good!

2021-04-07

Dario Righelli (08:34:22): - File (PDF): Screenshot 2021-04-07 at 14.33.59.pdf

Lukas Weber (13:42:12) (in thread): > updated slides from today - File (PDF): CZI_SpatialExperiment_LWeber_DRighelli_HCrowell.pdf

2021-04-09

Dario Righelli (04:57:43): > @Helena L. Crowellmaybe it’s easier if we talk about the colData drop here:smile:

Helena L. Crowell (04:58:11) (in thread): > Haha, yes, I keep getting emails every time there’s a comment on git lol

Helena L. Crowell (04:59:02): > I also feel that Slack’s getting messy sometimes and I have a hard time following… so just a heads up that I’ll try using threads if there’s a specific discussion about something:slightly_smiling_face:

Dario Righelli (05:00:00) (in thread): > I was just wondering about thecolDatadropping that drops also thespatialData, but at the same time thespatialData<-NULLdrops only thespatialData(obviously) > Maybe, we can drop thecolDatawithout dropping thespatialData, what do you think?

Helena L. Crowell (05:00:37) (in thread): > Aah, so I didn’t understand you earlier. You mean forcolData<-NULL?

Dario Righelli (05:00:43) (in thread): > yep

Helena L. Crowell (05:00:54) (in thread): > one sec, let me look at the code

Dario Righelli (05:01:41) (in thread): > x@colData[i] <- NULLthis instruction is dropping the entirecolDatastructure, which stores also thespatialData, is that right or am I missing something?

Helena L. Crowell (05:02:57) (in thread): > I totally agree!! And think that’s exactly what’s happening in thecolData<-method > > # protect spatialData from being replaced > spd <- spatialData(x, > spatialCoords=FALSE, > colData=FALSE) > value <- cbind(value, spd) > BiocGenerics:::replaceSlots(x, colData=value, check=FALSE) > > (basically I replace thecolDatabyspatialData- which is equivalent to removing allcolData, but keepingspatialData)

Dario Righelli (05:03:28) (in thread): > oh ok sorry, I didn’t see that code chunk!

Helena L. Crowell (05:03:45) (in thread): > forx@colData[i]<-NULL, we seti <- spatialDataNames(x)in the line above, so this drops only thespatialData

Helena L. Crowell (05:03:57) (in thread): > this is thespatialDatanotcolDatareplacement

Dario Righelli (05:04:17) (in thread): > also, I mislead to read the function where I was looking into…:cold_face:

Dario Righelli (05:04:33) (in thread): > yeah okokokokok, sorry again!:smile:

Helena L. Crowell (05:04:38) (in thread): > Actually, it might be easiest (instead of looking at the code) - to check some of the unit tests. Then you can see what YOU think should happen, and if it really happens that way:stuck_out_tongue:

Dario Righelli (05:05:00) (in thread): > good hint!:smile:

Helena L. Crowell (05:05:04) (in thread): > I tried covering all different scenarios there of dropping things

Dario Righelli (05:06:00) (in thread): > yes I saw that and thanks!

Dario Righelli (05:06:45) (in thread): > anyway I approved the PR:slightly_smiling_face:

Helena L. Crowell (05:09:05) (in thread): > Thx! I closed 2 issues - so I think now it’s down to documentation, and of course - I wrote all this pretty fast so we might run into minor changes along the way. But I’m hopeful the main structure will be left unchanged, and we might only have to make sure all the methods are robust.

Dario Righelli (05:11:52): > What about the remaining two opened issues? > Do we want to discuss them during our next meeting? Speaking with Levi about the next Bioc release and maybe this year will be around mid/end of May…

Helena L. Crowell (05:15:11) (in thread): > Yes, let’s discuss these next Tue. As well as making an action plan for what’s left - the release (usually) should happen end of this/next month. So it’d be good to get started asap in case so we can track down remaining issues / make sure everything is “intuitive” and well documented etc.

Dario Righelli (05:17:56) (in thread): > yes, usually it’s April, but there is no scheduled date for the R 4.1 release yet… > Anyway, I agree with you that we can go further on the issue solving process so to not rush when the deadline will come up.

Dario Righelli (05:18:26) (in thread): > Do we want to merge thespatialCoordsDatabranch into the master?

Dario Righelli (05:18:54) (in thread): > I think it’s mature enough, what do you think?

Helena L. Crowell (05:24:27) (in thread): > Hm, I think yes, the code itself is quite mature I believe, and all checks should pass. However, the documentation is completely out of line. So, I’d be fine merging intomasterso that we can branch-off from there the next couple weeks, but I wouldn’t put this on Bioc, as we’d get tons of documentation warnings at the moment, and it’ll be very confusing for the user to see documented parameters that no-longer exists in each function & the vignette…

Dario Righelli (05:26:43) (in thread): > oh yeah right, we discussed that once in master we should push on Bioc, so maybe we can wait for the documentation…

Lukas Weber (06:54:26) (in thread): > Ok, then I need to get onto documentation, sounds good:+1:

Lukas Weber (06:55:10) (in thread): > Oh, so Bioc release is delayed this round? Ok good to know

Lukas Weber (06:55:31) (in thread): > But yes normally would be around end of April

Helena L. Crowell (09:02:12) (in thread): > Let me know if you have any questions! Thecol/spatial/Data/Coordsmight be tricky in the beginning, but once you wrap your head around it & we present it well, I think (hope) it will be very intuitive & more robust than previous designs:slightly_smiling_face:

Helena L. Crowell (09:07:09) (in thread): > I’d say that’s still a possibility… I remember some time back they waited for the R release date, but then the Bioc release came 1-2 weeks after. So end of April / beginning of Mai is a deadline we should have in mind, regardless

2021-04-13

Helena L. Crowell (00:53:19): > May 4th it is:point_up:https://bioconductor.org/developers/release-schedule/

2021-04-14

Helena L. Crowell (03:20:37): > Hey all, > I’m really sorry about this - but in wanting to putTENxVisiumDataup for the next release, I noticed a bug inread10xVisium… basically, the counts & spatial data don’t match - so previously I was reordering one according to the other. But this assumes there are more counts than spatial coords or vice versa. So I didn’t run into this issue before, but am now when loading the 13 10x datasets… > > So just a heads up that I PRed a minor bug fix to instead do something more robust, i.e. > > obs <- intersect(rownames(spatialData), colnames(counts)) > spatialData <- spatialData[obs, ] > counts <- counts[, obs] > > so we can be sure this will work no matter what.

Dario Righelli (05:10:26): > ok thanks Helena!

Dario Righelli (05:22:16): > merged

Lukas Weber (10:12:05): > No worries, thanks! Interesting, yes previously I was also always assuming one is a subset of the other, and usingleft_join

2021-04-20

Dario Righelli (03:48:03): > Hi guys, have you received the Bioc2021 SpatialExperiment decision notification? - File (PDF): Screenshot 2021-04-20 at 09.47.07.pdf

2021-04-26

Dario Righelli (05:41:26): > Hi@Helena L. Crowell, I’m playing with the class a little and I noticed that when building the spatialCoords and spatialData from the colData it always stores everything in the colData. Is this a wanted behaviour? > > cd <- DataFrame(x=1:26, y=1:26, z=letters) > mat <- matrix(nrow=26, ncol=26) > spe <- SpatialExperiment(assay=mat, colData=cd, spatialCoordsNames=c("x", "y"), spatialDataNames="z") > spatialData(spe) > head(spatialCoords(spe)) > colData(spe) >

Helena L. Crowell (05:44:39): > hm… i guess not? the spatialData yes, spatialCoords no - I am surprised non of the unit tests caught this sorry…

Dario Righelli (05:53:49): > no problem, I was trying to put down the possible ways of building the object and I found this… I was not sure if you intended to do so or not:sweat_smile:

Dario Righelli (05:54:18): > I start a new issue, then we’ll decide how to handle it, sounds good?

Helena L. Crowell (05:55:58): > PR is also fine with me (including adding/fixing the appropriate unit tests?) & I can review that:slightly_smiling_face:- the spatialCoords should not go in the colData and stay separate as we already agreed, I think

Helena L. Crowell (05:56:19): > I think this only affectsread10xVisium- all else should be fine.

Lukas Weber (09:35:41): > Ah yes I think I was wondering about this too when I was working on the documentation (which I will try to finish asap) - so my understanding is we also have the additional argument too to show spatialCoords within spatialData: > > spatialData(spe, spatialCoords = TRUE) >

Lukas Weber (09:35:45): > and the default is FALSE - in which case it displays spatialData without the spatialCoords

Helena L. Crowell (09:53:21): > Yes, does this make sense to you or no? I think the reasoning is that spatialData and spatialCoords, by default, access the corresponding slots only. Meanwhile, spatialData can be used to return all of colData, spatialData, spatialCoords - but this it not the default

Lukas Weber (10:40:43): > Yes, I think it makes sense to have this argument. After thinking about it some more though, I think having default = TRUE would probably be better, since then users can just do a quickspatialData(spe)to get a snapshot of all the spatial stuff

Lukas Weber (10:41:10): > Right now users need to know that there is a difference betweenspatialDataandspatialCoords, which could make things slightly more difficult for beginners

Lukas Weber (10:41:13): > But I think either works really

Dario Righelli (11:00:22): > mmm maybe setting it to TRUE could be a little misinterpreting, because it’d be good to highlight the difference between spatialData and coords from the beginning… But it’s just my thought

Dario Righelli (12:26:47): > @Helena L. Crowell@Lukas WeberSorry, but I forgot to say that tomorrow I need to leave our meeting at 2:20, if this is a problem I have to ask you to reschedule it on Thursday or Friday. So sorry about that!

Lukas Weber (12:47:36): > No problem, either time is fine for me

2021-04-27

Helena L. Crowell (01:38:19): > Thu would be better for me - any time

Dario Righelli (03:19:54): > thanks guys! let’s re-schedule for Thursday then! :D

Dario Righelli (03:20:48): > I’m surely free after 3PM (9AM for Lukas)

Lukas Weber (07:39:27): > Ok, no worries - Thursday is a bit tricky - I am free only from about 1pm to 2:30pm (7pm to 8:30pm your time, so probably not good) > > Friday I am free any time from 9am onwards (3pm your time)

Dario Righelli (07:48:59): > Friday I’m free too, but it works also Thursday late afternoon… thanks guys!

Lukas Weber (07:50:53): > Ok thanks - let’s do Friday if Helena has time then - any time after 9am for me is good

2021-04-28

Dario Righelli (10:15:12): > Hi guys any updates on the meeting?@Lukas Weber@Helena L. Crowell

Dario Righelli (10:16:23): > I also received an email by the SubsetExperiment developers asking for our updates on the bioconductor repository, because they depend on our package. Do we want to schedule for an upload date? The deadline is approaching…:cold_face:

Lukas Weber (10:17:54): > Ah ok - yes I need to finish the doco pull request asap, sorry

Lukas Weber (10:18:10): > Would Friday meeting time work?

Lukas Weber (10:18:17): > Otherwise we can also just use the usual time but next week

Helena L. Crowell (10:18:27): > As I said before… Friday is not so good for me:confused:I will try to make it whatever time you decide, but not sure I can… Yeah, would be great to have the PR ready to discuss:slightly_smiling_face:

Lukas Weber (10:19:12): > Ah sorry, I missed that above

Helena L. Crowell (10:19:51): > Would 9am/15pm be an option? The later, the less likely I can join

Lukas Weber (10:20:03): > Yes 9am would be fine for me

Dario Righelli (10:20:15): > any time works for me guys and sorry again

Lukas Weber (10:20:17): > I have another meeting finishing 9am

Lukas Weber (10:20:37): > Ok then let’s do 9am / 3pm Friday, and if Helena can’t join, we can update here too

Lukas Weber (10:20:47): > Thanks

Dario Righelli (10:22:19): > ok it works for me! thanks!

Dario Righelli (10:55:06): > I created a google calendar invitation with a zoom link for Friday

Dario Righelli (10:55:16): > Let me know if you don’t receive it

Helena L. Crowell (10:59:18) (in thread): > Thanks, got it!

2021-04-30

Dario Righelli (06:57:30): > Hi@Helena L. Crowell, sorry to bother you, I just remembered aboutthis issueof Leonardo about theSamplevssample_idwhen using theread10xVisiumfunction. > I remembered that we already talk (and accounted) about this and indeed when I run the example theSamplefield is not present anymore. > Could you please confirm this?:slightly_smiling_face:Thanks

Lukas Weber (09:01:27): > I might be in wrong zoom

Dario Righelli (09:26:58): > I created the new meeting for the next Tuesday same time as usual (also inviting Stephanie and Davide for completeness) and sent everyone the Gcalendar invitation

Helena L. Crowell (09:37:36): > Just did a PR - the issue was not read10xVisium, but the SPE constructor - soSpatialExperiment.Rhas been modified, and I added a corresponding unit test following Dario’s minimal example from the GitHub issue.

Dario Righelli (09:43:03): > I just noticed this GHA problem that I really don’t know why, maybe the R-devel version is already 4.2 or something similar? > > Error: Error: Bioconductor version '3.13' requires R version '4.1'; R version is too new; see[https://bioconductor.org/install](https://bioconductor.org/install)Execution halted >

Dario Righelli (09:45:36) (in thread): > trying to fix it before accepting Helena’s PR

Dario Righelli (09:51:44): > ok GHA are broken I think, how do we proceed?:sweat_smile:

Lukas Weber (10:18:50): > Ah - yes, I saw this in some of the other channels, I think it may already be using R 4.2

Lukas Weber (10:18:57): > In this case maybe we just test locally for now

2021-05-01

Helena L. Crowell (07:27:45): > Minimal example of > * making up some molecule data > * re-shaping intoBumpyMatrix > * construct the SPE - File (Plain Text): molecules.Rmd

2021-05-03

Dario Righelli (03:10:19): > thanks!

Lukas Weber (07:59:06): > Thanks - I added this in the doc examples too. I set up the pull request for the docs - running some more checks now

Lukas Weber (07:59:41): > It is branched fromspatialCoordsData, but I don’t think there is any conflict with the other new one

Dario Righelli (08:58:49): > Thanks Lukas, I’m also finishing the vignette and checking for the tests… I’ll prepare a PR ASAP

Dario Righelli (09:22:12): > Sorry@Helena L. Crowellwhich invalid assignment do you want to enter in the vignette?

Dario Righelli (10:01:16): > I created the newPRwith the vignette to understand which parts I missed based on your suggestions (I’ve also created and closed a PR because on the wrong branch, sorry) and also because I found another issue#56

Helena L. Crowell (11:47:02) (in thread): > I was thinkin the ones that give an error in the unit tests… or at least one, two of them perhaps

2021-05-04

Helena L. Crowell (03:01:21): > Left some reviews on@Lukas Weber”s PR - sorry for the multiple comments, I basically went through each file and wrote a review. So the good news is I have no “obvious” comments on the files I didn’t comment on:slightly_smiling_face:

Helena L. Crowell (06:37:18): > Guys, I was pretty sick since Thu & just got an appointment at the doc - won’t be able to make 2… but I can approve the pr later today, maybe once my small comments are resolved? I’m sorry:disappointed:

Lukas Weber (07:54:25): > Ok, thanks for comments! I’ll have a look at them. No worries about the meeting - sorry to hear you are sick, I hope you get better

Dario Righelli (08:01:25): > No worries Helena, hope nothing bad! get well soon!

Dario Righelli (08:01:48): > Lukas are you connecting?

Lukas Weber (08:02:41): > Yes, just joined - maybe we can still meet briefly

Dario Righelli (08:03:33): > mmm maybe I’m using a wrong link …

Dario Righelli (08:04:20): > would you like to send me yours?

Dario Righelli (08:05:02): > dario.righelli@unipd.itis inviting you to a scheduled Zoom meeting. > > Topic: SpE Meeting > Time: May 4, 2021 02:00 PM Rome > > Join Zoom Meetinghttps://unipd.zoom.us/j/82284586667?pwd=dUFWSW5qZHpWcC9jVkRWUzUrOTNVQT09Meeting ID: 822 8458 6667 > Passcode: 971903 > One tap mobile+14086380968,,82284586667# US (San Jose)+16468769923,,82284586667# US (New York) > > Dial by your location+1 408 638 0968US (San Jose)+1 646 876 9923US (New York)+1 669 900 6833US (San Jose)+1 253 215 8782US (Tacoma)+1 301 715 8592US (Washington DC)+1 312 626 6799US (Chicago)+1 346 248 7799US (Houston) > Meeting ID: 822 8458 6667 > Find your local number:https://unipd.zoom.us/u/kbWwfrTRxJoin by SIP82284586667@zoomcrc.comJoin by H.323 > 162.255.37.11 (US West) > 162.255.36.11 (US East) > 115.114.131.7 (India Mumbai) > 115.114.115.7 (India Hyderabad) > 213.19.144.110 (Amsterdam Netherlands) > 213.244.140.110 (Germany) > 103.122.166.55 (Australia Sydney) > 103.122.167.55 (Australia Melbourne) > 149.137.40.110 (Singapore) > 64.211.144.160 (Brazil) > 69.174.57.160 (Canada Toronto) > 65.39.152.160 (Canada Vancouver) > 207.226.132.110 (Japan Tokyo) > 149.137.24.110 (Japan Osaka) > Meeting ID: 822 8458 6667 > Passcode: 971903

Dario Righelli (08:05:34): > I don’t understand what’s happening with my zoom

Lukas Weber (08:05:48): > Ok I’m in the one above now:joy:

Dario Righelli (08:05:55): > ok wait me there

Dario Righelli (08:05:56): > sorry

Dario Righelli (10:13:20): > Guys do you know any resource for visium artificially generated data?

Helena L. Crowell (10:16:37): > Resouces, no. But most of the deconvolution papers have some simulation part where they merge single cells into spots (a la pseudobulk) and sample spatial coordinates such that similar cell types are more likely to co-localize… or something along those lines if I remember correctly

Dario Righelli (10:17:51): > thanks Helena! How do you feel? is everything ok?

Helena L. Crowell (10:21:10): > Petty crappy, but the quick covid test was negative, so it’s probably just a flew… heading home now to lie down:sleeping:thx for checkin in:pray:

Lukas Weber (10:21:32): > Glad to hear - was worried it was covid

Lukas Weber (10:23:45): > On data - no, not really on artificially generated. For real data, one the best resources currently is probably the spatialLIBD dataset from our paper:bioconductor.org/packages/spatialLIBDandhttp://spatial.libd.org/spatialLIBD/

Dario Righelli (10:25:29): > thanks Lukas!

Lukas Weber (10:30:02): > and preprint on the package here:https://www.biorxiv.org/content/10.1101/2021.04.29.440149v1

2021-05-05

Lukas Weber (02:22:35): > Ok thanks for comments in the pull requests. I have added a few additional updates in the pull requests, and also added an additional pull request for the default “sample01” issue. > > If you are happy with these, then I think we can merge everything into master - i.e. first merge these pull requests intospatialCoordsData, then merge intomaster. > > Then to push to upstream, we will need to be careful between release and devel, since I believe the new release and devel branches have now already been created. The easiest way to do this will be to merge everything into both upstream branches (RELEASE_3_13andmaster, i.e. these are now the new release 3.13 and devel 3.14), and then add separate version bump commits in these two branches, then push both branches to upstream. This will avoid merge conflicts in the DESCRIPTION.

Helena L. Crowell (09:23:59): > Hey guys, if we’re all available today/tomorrow, should we try to wrap this up to give time in case the build fails?@Lukas WeberI left a comment on the sample ID PR, else I’m happy to merge the documentation PR - assuming the tests all pass. Do we know why GHA is breaking?

Lukas Weber (09:24:52): > Ok, sounds good! Just finishing a meeting now. I think GHA is breaking because it is already using R 4.2 - I saw some comments on this in one of the other channels

Lukas Weber (09:25:08): > So GHA won’t work properly again until after release

Lukas Weber (09:38:31): > Ok just updated the sample ID pull request usingsprintf- thanks for noticing that

Helena L. Crowell (09:51:04): > okay, so shall we merge the documentation one?

Helena L. Crowell (09:51:41): > we can always make changes after the release, especially to documentation, but think we should move forward so we have time to fix things if needed:slightly_smiling_face:

Helena L. Crowell (09:52:51) (in thread): > Thanks - Merged!

Lukas Weber (09:53:33): > Yep, I think merge everything intomasternow, then run checks again, then mergemasterinto the two new branches from upstream, then push

Lukas Weber (09:53:45): > That will give us a couple of days of build reports in case something unexpected is wrong

Lukas Weber (09:53:50) (in thread): > Thanks!

Lukas Weber (09:54:27): > Also when everything is in master, also add: (i) deleting old vignettes, (ii) separate version bumps in release and devel

Helena L. Crowell (09:54:28): > okay, I will PR into master and then ping yall to run checks - need approval to merge, then we can decide someone does the upstream push

Lukas Weber (09:55:17): > Maybe easiest if we add the deleting the two old vignettes as a new PR forked frommasterafter merging all the other branches

Lukas Weber (09:55:35): > Then run checks (withvignettes = TRUE), and then add version bumps

Helena L. Crowell (09:57:30): > Okay, PRed into master. I can run all checks etc. now

Lukas Weber (09:57:53): > Awesome, I’ll run them now too

Helena L. Crowell (10:00:04): > wait, let me do a PR to drop them vignettes, ok? Otherwise we’ll have to rerun checks again…

Lukas Weber (10:00:15): > ok thanks

Helena L. Crowell (10:01:09): > Also the seq fish data, yes?

Lukas Weber (10:01:31): > Yes, I think so

Helena L. Crowell (10:03:47): > okay, did a PR - once this goes thru we can check the spatialCoordsData branch for good, then merge into master

Lukas Weber (10:03:55): > ok will check it now

Dario Righelli (10:05:42): > mmm how does it works for the master<-spatialCoordsNames PR now? Does it automatically reflects new changes?

Helena L. Crowell (10:06:03): > okay - ready to check the crap out of all these PRs? lemme know how it goes:smile:

Helena L. Crowell (10:06:27) (in thread): > yes, the PR shows an update that we just did another push

Lukas Weber (10:06:33): > ok just pulled it, will run checks with vignettes=TRUE

Lukas Weber (10:06:58) (in thread): > yes I think GitHub tracks this automatically

Dario Righelli (10:07:14): > let me understand it better, do I pull from the spatialCoordsNames now?

Helena L. Crowell (10:08:41): > okay wow… this one”s big: > > $error > [1] ".Rbuildignore file includes the 'tests' folder." >

Helena L. Crowell (10:09:13) (in thread): > yes, exactly. it contains all the PRs (your vignette one, lukas documentation & sample ID, and my removing the old vignetes & data)

Lukas Weber (10:09:36): > error in vignette:The 'rmarkdown' package should be declared as a dependency of the 'SpatialExperiment' package (e.g., in the 'Suggests' field of DESCRIPTION), because the latter contains vignette(s) built with the 'rmarkdown' package.

Lukas Weber (10:09:39): > oh

Helena L. Crowell (10:09:41): > same.

Helena L. Crowell (10:09:48): > how did this pass earlier? I am confused

Lukas Weber (10:09:50): > I did not see that in .Rbuildignore

Helena L. Crowell (10:09:59): > ill push a fix, hold on…

Lukas Weber (10:10:26): > we had to skip vignettes checking in the local builds (vignettes=FALSE) since the old vignettes were no longer using valid objects

Helena L. Crowell (10:11:11): > also, why are we suggesting “Matrix”? Do we need tthis anywhere?

Lukas Weber (10:11:30): > we can probably remove that - it was previously required to create objects from scratch

Dario Righelli (10:11:48): > mmm I used it for loading the visium Matrix data, I guess we don’t need it anymore

Helena L. Crowell (10:11:52): > Okay, adding rmarkdown & dropping Matrix now.

Dario Righelli (10:12:02): > change also

Dario Righelli (10:12:09): > your role, just noticing that

Dario Righelli (10:12:26): > Lukas and yours

Helena L. Crowell (10:13:22): > okay done, it’s building now - but still fails! Thanks Dario, perhaps we can do that when we fix the versions? I’m sure we’ll need anothe few updates anyways:slightly_smiling_face:

Dario Righelli (10:13:28): > also should we need a maintainer or am I wrong?

Helena L. Crowell (10:14:15): > Okay… maybe this is more complicated than anticipated.@Dario Righellican you check that the vignettes are building, since this was your PR? Meanwhile Lukas & I can run all other checks

Dario Righelli (10:14:35): > sure!

Dario Righelli (10:16:01): > mmm how do I say to rmarkdown that we expect an error from a chunk?:sweat_smile:

Helena L. Crowell (10:16:16): > error = TRUE

Helena L. Crowell (10:16:22): > in the chunk options…

Dario Righelli (10:16:29): > perfect

Helena L. Crowell (10:17:34): > also, for me it’s fine if you push directly to spatialCoordsData if there are only minor fixes (as soon as the vignette builds)… we will still run checks on everything before mastering it:wink:

Lukas Weber (10:17:35): > Ah yes I see this one now - it gives an error as expected, so we need to tell rmarkdown to ignore this. Or put eval=FALSE

Helena L. Crowell (10:18:00): > biocccheck wont like eval = FALSE:wink:so id go with error = TRUE

Helena L. Crowell (10:18:10): > because this will also show the error message you uget

Dario Righelli (10:18:11): > ok it’s working now, do I push directly into the spatialCoordsData branch?

Lukas Weber (10:18:16): > yep

Helena L. Crowell (10:18:20): > yes

Dario Righelli (10:19:53): > done

Helena L. Crowell (10:20:35): > it built:christmas-parrot:

Lukas Weber (10:21:41): > awesome - checks are running now - I am running them on 2 systems (Mac and Linux), should take 2-3 min

Lukas Weber (10:24:44): > passed on Mac:christmas-parrot:

Dario Righelli (10:24:48): > I got 1 NOTE

Lukas Weber (10:24:52): > Linux will take a few more minutes since I was missing some dependency packages

Lukas Weber (10:25:00): > ah yes, I have one NOTE from the :::

Dario Righelli (10:25:03): > but it’s on the disableValidity

Lukas Weber (10:25:32): > if that is easy to fix then yes would be good to fix too - but it is only a NOTE

Dario Righelli (10:25:41): > never had so few notes:exploding_head:

Dario Righelli (10:26:20): > mmm who is gonna fix this? $error > [1] “.Rbuildignore file includes the ‘tests’ folder.”

Dario Righelli (10:26:51): > that’s a big problem: > > $warning > [1] "The following files are over 5MB in size: '.git/objects/pack/pack-1013329e988c30a40ed5f85b6f2f0787fac1a6c9.pack .git/objects/pack/pack-4c317a984fb5a102ce55be6c328865539744b30d.pack'" > [2] "Add non-empty \\value sections to the following man pages: man/SpatialExperiment-misc.Rd" >

Dario Righelli (10:27:41): > I don’t remember if it’s mandatory to pass all the BiocChecks during this phase

Helena L. Crowell (10:28:05): > @Lukas Weberdid the unit tests even run? I think until we remove that from the buildconfig they wont

Lukas Weber (10:28:39): > hmm yes this is possible - I didn’t realize this

Dario Righelli (10:28:44): > I can try to remove it now and test it

Lukas Weber (10:28:46): > same

Helena L. Crowell (10:28:58): > Yeah and someone push that pls :)

Dario Righelli (10:29:51): > Someone knows what is this? > > "Maintainer must add package name to Watched Tags on the support site; Edit your Support Site User Profile to add Watched Tags." >

Dario Righelli (10:30:34): > I’m trying right now to fix as many BiocChecks as possible

Helena L. Crowell (10:31:35): > I think this is a new addition to the checks - basically you should add a tag SpatialExperiment on your support site account (same email), but can do this later…

Helena L. Crowell (10:33:20): > generally, I don’t know where all this is coming from… i dont even have a build ignore other than for 1 special case where there were specific issues on windows… > > ^Meta$ > ^doc$ > #---------------------------- > # Git and SVN related > ^.svn > ^.git > ^/\.gitattributes$ > README[.]md > > # Travis-CI et al. > ^[.]travis[.]yml$ > ^travis-tool[.]sh$ > ^pkg-build[.]sh$ > ^appveyor[.]yml$ > ^covr-utils.R$ > ^[.]coveralls[.]R$ > # R related > ^cran-comments[.].*$ > ^vignettes/.*[.](pdf|PDF)$ > ^vignettes/.*[.](r|R)$ > ^vignettes/.*[.](html|HTML)$ > ^vignettes/[.]install_extras$ > ^Makefile$ > ^incl > ^NAMESPACE,.*[.]txt$ > ^nohup.*$ > ^[.]devel > ^[.]test > ^[.]check > ^.*\.Rproj$ > ^\.Rproj\.user$ > # DESCan2 > ^vignettes/DEScan2_cache > vignettes/coverage.html > # CPP builded > src/*\.o > # Other > testData > ^config\.status > ^config\.log > tests/testthat/pipeline\.R > inst/extdata/molecular_seqFISH/* >

Lukas Weber (10:33:25): > tests look ok

Lukas Weber (10:33:33): > I’ll delete the whole .Rbuildignore and replace with default > > ^.*\.Rproj$ > ^\.Rproj\.user$ > ^\.github$ >

Lukas Weber (10:33:38): > then run tests again

Lukas Weber (10:34:17): > (I ran tests by commenting out all thetestslines in .Rbuildignore just now, and they passed)

Dario Righelli (10:34:22): > I think that this is a Rbuildignore that I always copy and paste from a very old project

Helena L. Crowell (10:34:27): > anything that we dont want shouldn’t be on git anyways, so i think the simpler the build ignore the better

Helena L. Crowell (10:35:09) (in thread): > awesome - could you push (or PR if you prefer) the fixed / simplified .Rbuildignore?

Dario Righelli (10:35:11): > how do we proceed? I also checked and solved some stuff:sweat_smile:

Helena L. Crowell (10:35:29): > depends.. if its one-two lines, push. Else PR?

Lukas Weber (10:35:44): > I’ll run checks now, and then push the updated Rbuildignore - just deleting/replacing Rbuildignore

Dario Righelli (10:36:03): > mmm I’ll wait for you

Dario Righelli (10:36:10): > I think it’s better

Helena L. Crowell (10:38:12): > In other news… I’ll have to zone off pretty soon - not feeling well. Remember I said my test was negative? Turns out that’s what they call FNR… I lost my sense of smell and taste today and tested positive. Waiting for PCR results now, but chances are high there’s more Corona here than the ones in my fridge:weary:

Lukas Weber (10:38:57): > Oh I’m so sorry to hear that Helena - that does sound like it is likely corona

Lukas Weber (10:39:07): > oh you said positive

Lukas Weber (10:39:14): > really sorry to hear that

Helena L. Crowell (10:40:19): > Yeah, I hope it stays “calm”. mostly worried about others - i.e. I didnt know I had symptoms really for a while, then negative, then now this… So I could have conntributed to spreading it - that’s the tricky thing about the virus as they say:confused:

Lukas Weber (10:40:37): > Yeah, I hope you have a mild case

Lukas Weber (10:44:10): > Checks and tests passed - I made a new PR, so we can merge that intospatialCoordsData, then merge it all intomaster. Then any additional updates in DESCRIPTION inmasterbefore we start version bumps and pushing to upstream.

Lukas Weber (10:47:28): > If you are okay with it all now, Dario and I can take care of all these version bumps and making sure devel/release are synced up etc, and you can get some rest

Dario Righelli (10:51:55): > So sorry Helena, don’t worry, we can go ahead while you rest! Hope you’ll feel well soon!:four_leaf_clover:

Dario Righelli (10:53:03): > If it can help, I had a lot of friends with just smell and taste sense loss without any other symptom !

Lukas Weber (10:54:09) (in thread): > I made a new PR with just the Rbuildignore - can you approve it, then we can run checks again and merge into master I think

Dario Righelli (10:54:43) (in thread): > just approved

Lukas Weber (10:55:01): > Yes, so sorry to hear this Helena, please take some rest. Thanks for all your help today and yesterday. I hope you get better soon.

Lukas Weber (10:55:46) (in thread): > thanks, just merged - I’ll re-run checks, and then we can merge tomaster

Dario Righelli (10:57:19) (in thread): > There are some fixes from the biocchecks, I’ll take care of them, then you can work on the merge and on the upstream push (so we can also test this new “feature”)

Dario Righelli (10:57:28) (in thread): > what do you think?

Lukas Weber (10:57:55) (in thread): > ok thanks, sounds good - so you will push some fixes for BiocChecks tospatialCoordsData

Lukas Weber (11:01:59) (in thread): > We also need a NEWS entry - do you happen to have a quick list of the major changes in your mind?

Lukas Weber (11:02:18) (in thread): > And this will be version 1.1.700

Lukas Weber (11:10:38) (in thread): > I think we can fixThe following files are over 5MB in sizeafter we push today (i.e. try to fix it later this week), since this is already a warning in the current report

Helena L. Crowell (11:11:28): > Thx both for your understanding. Using this time to do some reading and writing etc - it helps just to not sit at the desk already and lie down instead:sleeping:

Davide Risso (11:12:36): > Sorry to hear@Helena L. Crowell! I hope you get well soon!

Dario Righelli (11:12:41) (in thread): > I’m getting this warning too it’s on the show method… > > [2] "Add non-empty \\value sections to the following man pages: man/SpatialExperiment-misc.Rd" >

Lukas Weber (11:13:03) (in thread): > Yes - do you want me to fix that? I think I see the problem

Davide Risso (11:13:43): > Sorry for being MIA these days, but if I can do anything to help with the Bioc submission let me know!

Dario Righelli (11:14:48) (in thread): > if you have time…

Dario Righelli (11:17:11) (in thread): > otherwise if you have the solution I can implement it

Lukas Weber (11:17:13) (in thread): > done - it didn’t like NULL

Lukas Weber (11:17:24) (in thread): > it is fixed now, you can pull it fromspatialCoordsData

Dario Righelli (11:17:45) (in thread): > thanks

Lukas Weber (11:20:02) (in thread): > did you have any other changes for BiocChecks? otherwise I think we can merge to master

Lukas Weber (11:20:42) (in thread): > oh and NEWS - maybe you can do that since you have the best overview of all the changes?

Dario Righelli (11:23:44) (in thread): > I had to edit just the DESCRIPTION file, I’m doing a push right now

Dario Righelli (11:24:00) (in thread): > Changed your roles and the biocviews as suggested

Dario Righelli (11:24:08) (in thread): > added the tag on my profile

Dario Righelli (11:24:49) (in thread): > done!

Dario Righelli (11:24:54) (in thread): > :tada:

Lukas Weber (11:25:03) (in thread): > :christmas-parrot:

Lukas Weber (11:27:11) (in thread): > could you add a quick NEWS entry for version 1.1.700?

Dario Righelli (11:27:46): > Thanks Davide! Anyway everything seems fixed, now Lukas is proceeding with branches merging and upstream push, we’re almost there! > Thanks again@Helena L. Crowellfor everything, get some rest and update us about your health status!:muscle:

Dario Righelli (11:27:58) (in thread): > oh sure!

Dario Righelli (11:28:14) (in thread): > Do I bump version?

Lukas Weber (11:29:08) (in thread): > no let’s do that separately

Lukas Weber (11:29:15) (in thread): > to minimize merge conflicts

Lukas Weber (11:29:55) (in thread): > oh could you please do NEWS and also add today’s date in DESCRIPTION - then for the version bump commit we only need to change a single line

Helena L. Crowell (11:30:55): > Just remember to do 1.1.600 when ur push as leo suggests- just to be safe:pray:

Lukas Weber (11:31:01) (in thread): > I have found previously that is the easiest way to minimize merge conflicts when doing things in both release and devel

Lukas Weber (11:31:09): > 1.1.600 or 1.1.700?

Dario Righelli (11:31:16) (in thread): > done

Lukas Weber (11:32:23): > I’ll do 1.1.700 since we already previously had 1.1.5 and 1.1.6 - thanks for the reminder

Lukas Weber (11:33:22) (in thread): > ok merge to GitHub master?:tada:

Lukas Weber (11:33:41) (in thread): > then I can try to sort out the release/devel and push to upstream if you are happy with that

Lukas Weber (11:34:01) (in thread): > oh hang on

Lukas Weber (11:34:56) (in thread): > I’ll add (i) today’s date in DESCRIPTION, and (ii) change 1.1.7 to 1.1.700 in NEWS, if this is okay with you - and I’ll push tospatialCoordsData

Lukas Weber (11:36:47) (in thread): > done

Lukas Weber (11:37:22) (in thread): > looks like there isn’t a RELEASE_3_13 branch yet, so it is actually easier than I thought - I’ll add a version bump inspatialCoordsData, then we can merge from there intomaster

Dario Righelli (11:38:23) (in thread): > approved

Lukas Weber (11:40:40) (in thread): > same - I’ll let you do the honours and click merge:tada:

Dario Righelli (11:43:20) (in thread): > aahahah thanks

Dario Righelli (11:43:56) (in thread): > done!:tada:

Lukas Weber (11:44:28) (in thread): > :tada:

Lukas Weber (11:44:31) (in thread): > just pushed to upstream too

Lukas Weber (11:44:34) (in thread): > :tada:

Lukas Weber (11:44:51) (in thread): > I think we’re done

Lukas Weber (11:45:12) (in thread): > there is no RELEASE_3_13 branch yet, so there was no issue of keeping release/devel in sync

Lukas Weber (11:48:08) (in thread): > re-running all checks again now just to double check

Lukas Weber (11:56:07): > No worries@Davide Risso- we have just finished the big update and pushed to both GitHub and Bioc-devel:tada:Thanks for all your help@Helena L. Crowell@Dario Righelli!! I hope you get better@Helena L. Crowell. > > Now during the rest of this week I will: > * check the build reports when they appear (1-2 days), in case there are unexpected issues > * try to figure out the >5MB file issue in .git history (BFG repo cleaner) > We can also add version bumps in our other packagesSTexampleDataandTENxVisiumDataso the build report uses the latest version of SPE.

Lukas Weber (11:56:26) (in thread): > all passed:tada:

Dario Righelli (11:56:50) (in thread): > perfect! Thanks a lot Lukas!

Lukas Weber (11:57:16) (in thread): > thanks!! nice push today to get it finished:tada:

Lukas Weber (11:57:25) (in thread): > I’ll keep a close eye on the build reports this week

Dario Righelli (11:57:31) (in thread): > yeah! nice “final” rush!:smile:

Lukas Weber (11:58:19) (in thread): > the fact that RELEASE_3_13 branch didn’t exist yet made things simpler, which was nice

Helena L. Crowell (12:13:33): > Awesome, thanks all for pulling through with this today! Thanks also for all the kind words and wishes, will keep you posted:crossed_fingers:

Lukas Weber (21:07:58): > I tested out this >5MB file size issue withBFG repo-cleaner/git filter-repoin a local clone on my laptop, and it looks like it may be simpler than I thought. The repo cleaner only detects large files in the non-master branches -masterbranch itself is already clean, so the commit hashes there don’t need to change if large files (e.g. >1MB) are stripped out everywhere. So this issue may only affect GitHub, not Bioconductor / upstream.@Dario Righellicould you try deleting any additional branches that you no longer need in the GitHub repo? There are several branches listed as “stale” (e.g. from workshops) - we probably don’t need these in the main repo (although I’m not sure, so wanted to check with you first). We can also delete all the merged ones. > > Then I can remove >1MB files from history in all remaining branches, and if it doesn’t change any commit hashes in themasterbranch, we can just update GitHub and we are done - no changes to Bioconductor / upstream needed.

Lukas Weber (21:24:26): > If we strip files >1MB then it removes the following files from history, which do not affect the latest commit hashes inmasterbranch: > > Filename Git id > ----------------------------------------------------------------- > matrix.mtx.gz | 2041b6bd (57.9 MB), 9d7c31cd (64.4 MB) > myve.rda | cecae854 (2.1 MB) > raw_feature_bc_matrix.h5 | fd605e15 (22.3 MB), db73f965 (24.6 MB) > ve.rda | 88309199 (1.2 MB), 73026130 (2.8 MB) > > But we can also go further down, e.g. >50K and *.html, then it removes a lot more and requires overwriting latest commit hashes. Maybe we can first get rid of those four files above if that is simple and solves 90% of the problem.

2021-05-06

Dario Righelli (04:01:31): > Hi Lukas, I’d prefer to leave the workshop branches there at the moment, some of them are related to papers or to bioc workshops. I hope this is not a problem. > About the newest “devel” branches I’ll surely delete them ASAP…

Lukas Weber (09:13:19): > Ok, no problem!

Lukas Weber (09:14:20): > I’ll check if the changed commits for removing the large objects from history would be in those branches or other ones

Lukas Weber (13:10:17): > Build report passed - all green:tada:http://bioconductor.org/checkResults/devel/bioc-LATEST/SpatialExperiment/

Lukas Weber (13:10:34): > Version 1.1.700

Lukas Weber (13:14:48): > @Dario Righellido we need thegh-pagesbranch? I’m not sure what is in that one. (There is also the wiki, but that is in a separate wiki repo.)

Lukas Weber (13:30:24): > Ah ok I understand the >5MB issue now. The large files in history are all ineurobioc2020branch. So I think there are two relatively easy ways to fix this: > > (1) I push a “BFG repo cleaned” version that overwrites the history and commit hashes ineurobioc2020branch - but I’m not completely sure if this will cause problems for you later if you re-use this branch@Dario Righelli(2) Alternatively,@Dario Righelliyou could make a separate clone of the whole repo (e.g. call itdrighelli/SpatialExperiment-workshops) where you keep these branches (eurobioc2020,BIRSBIO20_seqFISH,gh-pages) in their original state (30 MB or so), and we delete these branches from the main repo. > > I think my preference would be (2), since this makes things simple for any users in the future who clone and fork the main repo, since there are less branches in it and they don’t need to think about what the workshop branches are for.@Dario Righelliwould this work for you? Alternatively if you strongly prefer option (1) I can also do this.

2021-05-07

Dario Righelli (03:39:04) (in thread): > this is needed for the gh (GitHub Actions)

Dario Righelli (04:24:58): > Hi Lukas, thanks for looking into this, but the problem with those branches is that they are linked in some other repositories (not mine) and I’m not able to remember them… > So if I move them into another repo, if ever (maybe never) someone will look for them they will not be able to retrieve them. > In particular I’m worried about theBIRSBIO20_seqFISH. Thegh_pagescannot be moved because it is for theGHA. > For theeurobioc2020branch I suppose that we can modify it, but of course it will be good to have it working.

Lukas Weber (08:41:55): > Ah ok - no worries. In that case option (1) is probably better. I’ll try it out in a fork in my own GitHub first. All branches should still work, just some of the commit hashes ineurobioc2020branch will change due to modified history. > > I just found out the whole git repo (all branches) is stored in the Bioc git, so we are currently using up quite a lot of space there (~100 MB) - so it would definitely be good if we can get it down to a few MB instead. > > (If we do eventually move the branches to a new repo, we could also leave a note on the Readme in case someone is looking for them in the future - but I’ll try option (1) first, so we don’t need to move them.)

Lukas Weber (08:53:34) (in thread): > ah ok, thanks!

Dario Righelli (09:48:47): > Thanks!

Lukas Weber (15:01:08): > Ok it’s fixed now, with option (1). So the workshop branches are still there in the repo, which I think is okay. The repo is now only 4.7MB with the large files removed from history ineurobioc2020branch (instead of >100MB previously). I also left a backup of this branch in my GitHub atlmweber/SpatialExperimentin case I messed something up. Themasterbranch is unaffected, so we don’t need to contact the Bioc team.

Lukas Weber (15:02:32): > I also noticed theRELEASE_3_12branch in GitHub is out of sync withRELEASE_3_12on Bioc, so I’ll replace it on GitHub as a historical record. Pushes toRELEASE_3_12are disabled now anyway according to the release schedule, so that branch is now obsolete anyway.

Lukas Weber (15:05:08): > Ok all fixed now and a nice clean repo:tada:

Lukas Weber (15:07:11): > There are also some other un-needed files in history in the 50K-1MB size range (e.g. .txt data files we added and deleted), which we could also technically remove from history if wereallywanted a tiny/clean repo - but these would require overwriting commit hashes inmasterbranch and hence contacting Bioc team, so I think this is not worth it. We already went from 117 MB -> 4.7 MB with the easy changes above, so I think this is good enough:slightly_smiling_face:

Lukas Weber (15:24:53): > So I think we are done for now. Thanks everyone!!:tada::tada:

Lukas Weber (15:27:27): > If any small additional bugfixes come up as I work with the objects in my projects, then I will prepare these as small additional pull requests againstmasterbranch as they arise:party_parrot:

2021-05-10

Dario Righelli (03:33:58): > Great job Lukas! Thank you very much!!:smile:

Dario Righelli (03:45:43): > How do you test the dimension of the repo?

Lukas Weber (09:14:06): > I just clone it and dodu -shon the directory. If you clone withgit clone --mirrorit getseverything(including historical pull requests, and all branches). If you do standardgit clonethen it is what most people will get.

Dario Righelli (09:57:13): > Leonardo opened aPR, I’ve already reviewed it, but I’d like your opinion too…

Lukas Weber (10:11:54): > Ok, will have a look at it later today

Lukas Weber (14:20:26): > I had a look - looks fine, it’s just adding an argument togrep. I ran checks and it all still passes. There are also some changes to end-of-line spacing in the documentation block, sodevtools::document()needs to be re-run - I have asked Leo to add a commit with this, since the pull request is in his GitHub

Lukas Weber (14:27:29): > Also@Dario Righelliwe can skip tomorrow’s meeting if you are busy. We could also try another when2meet for next week if this time is not good for you generally. Generally the 8am / 2pm slot is good for me (although not Wednesday or Friday right now)

Dario Righelli (14:38:06): > sounds good to me to have a new when2meet

Dario Righelli (14:38:33) (in thread): > yes I saw the changes, it’s always good to have another couple of eyes!:slightly_smiling_face:

Dario Righelli (14:38:47) (in thread): > thanks!

Lukas Weber (14:38:54) (in thread): > :+1:

Lukas Weber (14:39:17) (in thread): > actually you could also just merge it now, and I’ll add a second PR with thedevtools::document()so Leo doesn’t need to do it

Lukas Weber (14:39:30) (in thread): > ah he just added it

Lukas Weber (14:41:23) (in thread): > ok I merged it

Lukas Weber (14:41:37) (in thread): > I’ll double checkdevtools::check()again just in case

Lukas Weber (14:48:08) (in thread): > everything still passes - I’ll push it toupstream

Lukas Weber (14:49:18) (in thread): > done

2021-05-11

Dario Righelli (05:26:17) (in thread): > great! thanks again Lukas!

Davide Risso (08:03:33): > Just confirming that we are not meeting now, correct?

Lukas Weber (08:38:12): > Ah sorry, wasn’t on Slack - correct, no meeting today

Megha Lal (16:46:00): > @Megha Lal has joined the channel

2021-05-13

Aedin Culhane (17:25:01): > @Aedin Culhane has joined the channel

2021-05-20

Lukas Weber (16:13:41): > We have a minor issue where we are getting duplicated columns introduced incolDatawhen adding new columns with external tools such asscater::addPerCellQC. I think it is because after adding any new columns tocolData, it automatically wrapsspatialDataat the end again. I’ll think about how best to solve this, and open and issue and pull request / unit test.

Lukas Weber (16:14:24): > Currently my hack is to do this after runningscater::addPerCellQC > > # note: remove duplicated columns introduced when using scater > colData(spe) <- colData(spe)[, !(duplicated(colnames(colData(spe))) | duplicated(colnames(colData(spe)), fromLast = TRUE))] >

Lukas Weber (16:14:44): > and then at the end it re-wraps thespatialDatacolumns fresh

Lukas Weber (16:15:46): > Possibly one way to solve it would be to have an argumentcolData(spe, spatialData = FALSE)by default, so these columns are not included incolDataby default. This would be similar to thespatialData(spe, spatialCoords = FALSE)argument

Lukas Weber (16:15:55): > I’ll move this discussion into a GitHub issue and we can discuss on Monday

Lukas Weber (16:30:26): > Opened an issue so we can keep track of the discussion

2021-05-24

Lukas Weber (12:08:44): > I added a copy of the new branchRELEASE_3_13inoriginso we can use this as the base for pull requests for bug fixes that need to go into release version

Lukas Weber (12:16:52): > @Dario RighelliI added a small pull request just now with the version bumps for the Bioconductor release. These version bumps were added by the Bioc team inupstream, so we need to merge them intoorigintoo. Could you please review this PR, and then I will continue with the PR for the duplicated columns issue. Thanks!

Dario Righelli (12:18:24): > thanks, reviewed and merged!:wink:

Lukas Weber (12:18:45): > Awesome thanks

Lukas Weber (16:01:02): > Ok I have tried to fix it but am stuck - I posted my progress so far in the GitHub issue

2021-05-26

Dario Righelli (09:41:56): > Hi@Lukas WeberI wanted to know if you’re still working on the issue or if you prefer that I take care of it.

Lukas Weber (09:47:51): > Hey - I was planning to continue with it today or tomorrow, but if you can see the solution / have time today then I would definitely be happy for you to continue it!

Dario Righelli (09:50:12): > I’m not sure about the solution, but I have some ideas… Anyway I’m not sure if I’ll be able to work on it today and maybe not before Monday…

Lukas Weber (09:51:28): > Oh, no worries - ok, I’ll try too - I’ll post something in the github issue if I figure something out

Dario Righelli (10:09:19): > thanks

2021-06-03

Dario Righelli (10:06:17): > spe updated figure - File (PNG): SpatialExperimentSplitted0.2.png

Dario Righelli (10:17:47): > Just to keep you updated: about the issue, I fixed the colData copying, but we got another problem during the multiple samples cbind.

Lukas Weber (21:57:36): > Oh, awesome, thanks! I was stuck with it

Lukas Weber (21:57:50): > And thanks for figure too

Lukas Weber (21:57:58): > Will test this out

2021-06-04

Dario Righelli (02:50:43): > I’ll work on it today and then I’ll push the updates

Lukas Weber (08:58:37): > Ok, that sounds great

Dario Righelli (09:00:46): > Hi guys, I have a question about thespatialData/colDataassignment. > at the momentinto the colDatawe have an instruction for protecting thespatialDatafrom being replaced > > # protect spatialData from being replaced > spd <- spatialData(x, > spatialCoords=FALSE, > colData=FALSE) > value <- cbind(value, spd) > > butinto thespatialDatawe call thecolDatato assign the newcolData > > colData(x) <- cbind(colData(x)[cd_keep], value) > > I understand the meaning of the first instruction, but when we want to assign the spatialData we fall into some sort of loop…

Dario Righelli (09:47:35): > ok I solved in another way, I’ve just added a check on this instruction into thecolDatareplacement > > spd <- spatialData(x, > spatialCoords=FALSE, > colData=FALSE) > if (!all(spatialCoordsNames(x) %in% colnames(value))) { > value <- cbind(value, spd) > } >

Dario Righelli (09:47:51): > I’m going to push the changes so you can check the code

Dario Righelli (09:49:45): > @Lukas Weberbefore proceeding with the PR would you like to check the code?https://github.com/drighelli/SpatialExperiment/tree/colData_duplicate_cols

Lukas Weber (10:17:36): > Ok great, will have a close look at it later today, thanks a lot!

Lukas Weber (16:23:42): > Hmm@Dario RighelliI don’t think this worked - I just tested it out, and I still get the duplicated columns. I have pasted an example and output in the GitHub issue.

Lukas Weber (16:31:52): > However if I change your new code block to checkspatialDataNamesinstead ofspatialCoordsNamesI think it works. Will test it some more and then push a change to the same branch

Lukas Weber (17:18:06): > I made a pull request for you to review. I think it is working now:tada:

Lukas Weber (17:19:45): > I also added a unit test (and commented out Bioc 3.14 from the updated GitHub Actions for now, since this one is failing in GHA - maybe it is still too new in GHA)

2021-06-06

Dario Righelli (11:01:16) (in thread): > oh thanks!!

Dario Righelli (11:37:54): > thanks I proceed with the merging on the master, then we’ll decide how to proceed with Bioc

Lukas Weber (11:55:58): > Ok awesome. Do you think we should put this one in both release and devel? If so, I cangit cherry-pickthe comments across into release branch too (along with a separate version bump). Then we can push to bothupstream masterandupstream RELEASE_3_13.

Lukas Weber (11:56:33): > Probably makes sense in this case if it is only a bug fix

2021-06-08

Dario Righelli (03:07:49): > I think this bug is relevant enough to justify the push on the RELEASE_3_13

Dario Righelli (03:08:32): > Then before proceeding with other development (if any) I think it could be nice to make a RELEASE_3_13 branch on the github

Lukas Weber (10:11:35): > Ok thanks, sounds good. I have usedgit cherry-pickto update the branchRELEASE_3_13on GitHub (origin) just now, and added a version bump. > > If there are no objections, I will go ahead and push bothmasterandRELEASE_3_13from GitHub (origin) to Bioconductor (upstream).

Dario Righelli (15:32:44): > ok thanks!

Lukas Weber (16:28:54): > ok pushed it to both:+1:

Lukas Weber (16:29:12): > I’ll check the build reports over the next 1-2 days to check nothing went wrong

Lukas Weber (16:33:27): > I have also opened this issue so we can think about the defaults for the show methods forcolData,spatialData, andspatialCoords. I think changing the defaults in this way would make things more intuitive for users, since each of the 3 slots is then clearly shown as a subset of the next (and they can easily be combined withcbindto pipe to ggplot etc, or alternatively changing the arguments to TRUE):https://github.com/drighelli/SpatialExperiment/issues/69

2021-06-10

Lukas Weber (21:21:41): > devel build report is green:tada:

Lukas Weber (21:21:46): > release build report hasn’t come in yet

2021-06-14

Dario Righelli (03:11:39): > Morning guys, do we have our meeting today?

Lukas Weber (08:31:34): > Yes, I think so. Updates from my end are: > * our last set of updates to fix the duplicated columns issue has passed on build reports (thanks@Dario Righellifor figuring out the solution!!):tada: > * we can discuss the issue in my message above (June 8th) on default show methods > * I will plan to reformat the paper asap so we can resubmit

2021-06-17

Dario Righelli (15:45:39): > Someone knows why the build status on Bioconductor is unknown?https://bioconductor.org/packages/release/bioc/html/SpatialExperiment.html - Attachment (Bioconductor): SpatialExperiment > Defines S4 classes for storing data for spatial experiments. Main examples are reported by using seqFISH and 10x-Visium Spatial Gene Expression data. This includes specialized methods for storing, retrieving spatial coordinates, 10x dedicated parameters and their handling.

Lukas Weber (16:23:38): > Hmm, no, not sure. It’s passing all the build reports in release and devel. Maybe this badge is out of date

Stephanie Hicks (16:46:30): > is it the new knitr error Lori mentioned?

Stephanie Hicks (16:46:48): > like all of my packages now have this error and i’ll have to deal with it asap

Lukas Weber (16:49:54): > No, this one is still passing. I had the error Lori mentioned inSTexampleDatathough - it can be fixed by addingrmarkdownto Suggests in DESCRIPTION

Lukas Weber (16:52:13): > I think it is just the badge on the landing page is broken (forSpatialExperiment)

2021-06-18

Dario Righelli (04:30:57): > Indeed, I was on the SpE page for checking the “Lori’s Error”:sweat_smile:

2021-06-21

Lukas Weber (08:51:36): > Just pasting message here instead of in DMs - here is a quick summary of our current work items: > * update paper in new format (Lukas) > * issue on how to update show methods / getters, as described here:https://github.com/drighelli/SpatialExperiment/issues/69(if either Dario or Helena has time to work on this that would be wonderful) > * (previous issue on duplicated columns in colData is solved and merged) > * update / check associated packages (ggspavis and TENxVisiumData) to make sure everything works with new SPE structure > * prepare for Bioc

2021-06-22

Helena L. Crowell (02:40:19): > I can work on issue #69 now/today if that’s okay?

Dario Righelli (02:59:49): > it works for me

Helena L. Crowell (04:34:38): > Okay, not as straightforward as I had thought… could use some thoughts from both of you- see my comment on GH (not sure I explained it well, it’s all a bit recursive:confused:)

Dario Righelli (04:52:15): > Thanks, It’s really clear, at least to me… Maybe because I’ve already tried to implement it for another issue…

2021-06-28

Helena L. Crowell (07:52:03): > We’re meeting today at 9/15 correct?

Lukas Weber (08:27:07): > Hey - sorry for last minute, I can’t make it today. I am trying to madly finish some other paper revisions.

Dario Righelli (08:47:18): > Do we want to meet anyway or do we want to meet next week?

2021-07-06

Helena L. Crowell (05:12:44): > * updates in branchspatialData > * R CMD check&BiocCheckpass (for me) > * before we PR & review, the vignette needs updating (@Dario Righelli)? > * then probably best we all review & give our okay before merging

Dario Righelli (05:14:00): > thanks Helena, I’ll work on the vignette and will update here… I’ll work on the same branch ok?

Dario Righelli (05:21:32): > which version of R/Bioc are you using?

Dario Righelli (05:33:39): > i’m getting this error: > > > img <- imgRaster(spe, > + sample_id = "section1", > + image_id = "pomeranian") > Error in (function (cache = getBFCOption("CACHE"), ask = interactive()) : > DEFUNCT: As of BiocFileCache (>1.15.1), default caching location has changed. > Problematic cache: /Users/inzirio/Library/Caches/BiocFileCache > See[https://www.bioconductor.org/packages/devel/bioc/vignettes/BiocFileCache/inst/doc/BiocFileCache.html#default-caching-location-update](https://www.bioconductor.org/packages/devel/bioc/vignettes/BiocFileCache/inst/doc/BiocFileCache.html#default-caching-location-update)Calls: imgRaster ... FUN -> .remote_file_cache -> do.call -> <Anonymous> > Execution halted >

Dario Righelli (05:43:34): > Do you think it’s really necessary to add a vignette section to specify where the things are stored?

Helena L. Crowell (05:53:42) (in thread): > Bioc 3.14 - the devel version. But checks also went fine on release for me…

Davide Risso (05:57:41): > @Dario RighelliI think that this error was something Lori mentioned a while ago. IIRC manually emptying the cache should solve the error

Dario Righelli (05:57:56) (in thread): > Do you know where the cache is typically stored?

Helena L. Crowell (05:57:56): > 1. Yes, I think it is. Since we’re extending the SCE, it’ worth briefly mentioning which slots we add & how/where they’re stored. I had mentioned this previously for both thespatial&imgData(it’s been removed though…) But this is open to discussion / personal preference I guess! > 2. Also, now that I look at it, I am thoroughly confused (not in a good way)… I had written an extensive section on theimgData, including associated functions (add/rmvImgetc.), caching, loading into memory… This is all gone now, and there is no mention whatsoever on how to access/manipulate theimgData? > 3. Also no mention of theSpatialImageclass, which is an entire class with sub-classes we implemented. Where did all this go?

Dario Righelli (06:00:00): > I remember that section… but I don’t know what happened to it… But I’m sure we can retrieve it from previous commits

Helena L. Crowell (06:00:53): > Yes, we should… I’m really surprised to just notice this now, but it’s not good.

Dario Righelli (06:01:40): > I thought it was there…

Helena L. Crowell (06:05:13) (in thread): > It’s says it in the error message no?

Dario Righelli (06:05:39) (in thread): > oh you’re right!:man-facepalming:

Dario Righelli (06:06:39): > Is this it? I retrieved this file from the history - File (R): imgData-methods.R

Davide Risso (06:07:38): > It seems that there was a section in the older vignette, which was not ported in the new one?

Davide Risso (06:07:39): > https://github.com/drighelli/SpatialExperiment/commit/266b670024ee4a62092740c1c4caa4edcb09b3f9

Helena L. Crowell (06:08:24): > @Dario Righellino, that’s just the R doc which is still there (otherwise we’d get documentation errors for sure). > Yes, exactly, the previous vignette was much more extensive, and it looks like many parts just got lost (not just image-related, but overall it’s much briefer now…).

Stephanie Hicks (06:33:50): > Hi everyone! I’m back from leave starting this week. Just giving a short announcement. :)

Stephanie Hicks (06:34:26): > Unfortunately I can’t make next week’s proposed meeting on Monday because I’m giving a virtual presentation in the UK at the same time

Stephanie Hicks (06:35:37): > I’m more than happy to chat on here (or another time during the week) about plans for getting the paper resubmitted.

Dario Righelli (06:40:21) (in thread): > Oh ok, I think we spoke about leaving the vignette shorter, but maybe I’m wrong!:sweat_smile:I can add the imgStuff to the vignette during this implementation phase…

Davide Risso (06:42:29): > Welcome back@Stephanie Hicks!:slightly_smiling_face:

Davide Risso (06:43:41): > I actually have an exam at the time of our usual meeting next Monday, so perhaps we can find an alternative day just for next week? Or simply meet in two weeks as the original plan…

Stephanie Hicks (06:46:01): > In two weeks on the 19th works for me (or another time too). I’m also happy to do some editing / writing before then as prep work before the meeting

Helena L. Crowell (07:29:20) (in thread): > Sure, shorter is fine. But we should still cover all novel parts, i.e. theimgDatamethods &spatialImageclass

Dario Righelli (09:38:14) (in thread): > is it ok if I take this part from the 10x visium vignette? > ``> # Image data > > All image related data are stored inside theint_metadata'simgDatafield asDataFrameof the following structure: > > * each row corresponds to one image for a given sample and with a given unique image identifier (e.g. its resolutions) > * for each image, columns specify: > * whichsample_idthe image belongs to > * a uniqueimage_idin order to accommodate multiple images for a given sample (e.g. of different resolutions) > * the image'sdata(aSpatialImageobject) > * thescaleFactorthat adjusts pixel positions of the original, full-resolution image to pixel positions in the image > > TheimgData()` accessor can be used to retrieve the image data stored within the object: >

imgData(spe)

The SpatialImage class

Images are stored inside the data field of the imgData as a list of SpatialImages. Each image may be of one of the following sub-classes:

• LoadedSpatialImage
  • represents an image that is fully realized into memory as a raster object
  • @image contains a raster object: a matrix of RGB colors for each pixel in the image
• StoredSpatialImage:
  • represents an image that is stored in a local file (e.g., as a .png, .jpg or .tif), and loaded into memory only on request
  • @path specifies a local file from which to retrieve the image
• RemoteSpatialImage
  • represents an image that is remotely hosted (under some URL), and retrieved only on request
  • @url specifies where to retrieve the image from

A SpatialImage can be accessed using getImg(), or retrieved directly from the imgData():

(spi <- getImg(spe))
identical(spi, imgData(spe)$data[[1]])

Data available in an object of class SpatialImage may be accessed via the imgRaster() and imgSource() accessors:

{fig.small = TRUE} plot(imgRaster(spe))

Adding/removing images

Images entries may be added or removed from a SpatialExperiment’s imgData DataFrame using addImg() and rmvImg(), respectively.

Besides a path or URL to source the image from and a numeric scale factor, addImg() requires specification of the sample_id the new image belongs to, and an image_id that is not yet in use for that sample:

url <- "[https://i.redd.it/3pw5uah7xo041.jpg](https://i.redd.it/3pw5uah7xo041.jpg)"
spe <- addImg(spe, 
    sample_id = "section1", image_id = "pomeranian",
    imageSource = url, scaleFactor = NA_real_, load = TRUE)
img <- imgRaster(spe, 
    sample_id = "section1", 
    image_id = "pomeranian")
plot(img)

The rmvImg() function is more flexible in the specification of the sample/image_id arguments. Specifically,

• TRUE is equivalent to all, e.g. sample_id = "<sample>", image_id = TRUE will drop all images for a given sample.
• NULL defaults to the first entry available, e.g., sample_id = "<sample>", image_id = NULL will drop the first image for a given sample.

For example, sample_id,image_id = TRUE,TRUE will specify all images; NULL,NULL corresponds to the first image entry in the imgData; TRUE,NULL equals the first image for all samples; and NULL,TRUE matches all images for the first sample.

Here, we remove section1’s pomeranian image added in the previous code chunk; the image is now completely gone from the imgData:

imgData(spe <- rmvImg(spe, "section1", "pomeranian"))

```

Dario Righelli (10:33:54): > I just pushed the updated vignette

Helena L. Crowell (11:01:20): > …needed for a document I have to fill out (that I feel is entirely stupid & against my principles)… where do we plan to submit the SPE paper?

Davide Risso (11:26:38): > I think the idea was a Bioinformatics application note

Stephanie Hicks (13:54:00): > for the italian and swiss crowd:https://twitter.com/raphg/status/1412409686666760200?s=20 - Attachment (twitter): Attachment > We’ll soon be looking for postdocs/students, data scientists and programmers. #RStats @Bioconductor We are building a new biomedical data science center bridging the hospital, the university and other centers in Lausanne. More details will be shared later. If interested DM me.

2021-07-07

Dario Righelli (03:56:17): > Thanks Stephanie!

2021-07-12

Lukas Weber (09:18:35): > Hope everyone had a nice weekend! My SpatialExperiment to-do list this week: > * SpatialExperiment paper reformatting > * check all associated packages referred to in paper work correctly with new SPE objects (ggspavis etc) > * start preparing OSTA workshop for Bioc conference

2021-07-19

Lukas Weber (08:56:01): > To discuss in our meeting today: > * updated paper is ready for feedback > * need to check/update associated packages (STexampleData, TENxVisiumData, ggspavis) > * prepare workshops for Bioc

Lukas Weber (09:24:41): > Quick update from our discussion: > * everyone to give feedback on paper by Friday > * check/update associated packages (STexampleData, TENxVisiumData, ggspavis) by Friday > * Dario, Lukas, Helena to review and test updated structure and vignette examples in spatialData branch from Helena (structure) and Dario (vignette) by Friday > * meet again next Monday; weekly meetings until Bioc to discuss paper submission and Bioc workshop > * plan to submit paper next week

Lukas Weber (09:37:26): > Also quick update on the main changes in the paper for@Davide Risso > * shortened to meet length requirements for the new format (1000-1300 words / 1 figure) > * moved two panels of the figure to Supplementary Figures, so now the figure in the main paper only shows the SPE structure (the schematic Dario previously made) > * moved table on STexampleData datasets to Supplementary Tables > * add second Supplementary Table on TENxVisiumData datasets

2021-07-21

Helena L. Crowell (02:27:57): > Quick Q: > * I’m aware the guidelines area == bvs.fun(a=1, b=2, ...) > * However, I personally prefer having > > > sce <- SingleCellExperiment( > assays= list(...), > colData = DataFrame(...)) > > and similar in vignettes, for readability. > * Currently, we are inconsistent, e.g. sometimesa=1, sometimesa = 1, sometimes mixingfun(a = 1, b=2, ...) > * Please vote for:one:with spaces around=or:two:no spaces > * Either way, I’d be happy to go through & make things consistent once we have a vote:slightly_smiling_face:

Lukas Weber (12:55:26): > I prefer spaces around equals signs - thanks for noticing this

Lukas Weber (12:56:02): > And definitely is good to be consistent throughout

Helena L. Crowell (13:06:23): > Great, thx all. I’ll go thru this and PR along with other stuff

2021-07-22

Helena L. Crowell (03:11:00): > Hey all, > I am going through the vignette at the moment, and have many thoughts… But am hesitant to change everything without some discussion. Specifically, I think the current structure is quite messy in that we have somewhat unrelated headings in no particular order. I think having something like the following structure would be desirable: > > The SPE class > spatialData & spatialCoords > Subsetting SPE objects > Combining SPE objects > sample_id replacement > imgData > The SpatialImage class > Adding/Removing images > Constructing a SPE > Manually (-> SpatialExperiment) > Spot-based (-> read10xVisium) > Molecule-based (-> BumpyMatrix) > Session Info > > Alternatively, the SCE vignette has an additional distinction between structure & operations, which I’d even prefer. Something like > > The SPE class > spatialData & spatialCoords > imgData > The SpatialImage class > Adding/Removing images > Common operations > Subsetting SPE objects > Combining SPE objects > sample_id replacement > > In comparison, we currently start with spatialData/Coords were we also go through the construction, then combining, subsetting, replacement, then spot-based, then image data, then molecule-based… which is, as I said, not so intuitive for me to read. Ideally, I think we should start with a description of the overall structure (brief), separately show the different ways of using the constructor, and then finish with 10x vs. molecule-based specific construction. > > Currently (all 1st level headers): > > The SPE Class > SpatialData & spatialCoords (constructor is explained in here) > Multiple Samples > Subsetting > Replacement > Spot-based > imgData > Molecule-based > > Also, once we finalise it, could we add the schematic in the vignette or what do you think?

Dario Righelli (04:13:54): > either new proposed structures work for me, thanks for looking into that

Stephanie Hicks (22:29:36): > i like your second one@Helena L. Crowell— structures and then operations

2021-07-23

Helena L. Crowell (03:17:11): > Okay, so I can give the restructuring a try. This might require adapting / rearranging code chunks. I’ll PR a copy, so we keep the original vignette for now to make comparison easier (GH compare might become messy)

Helena L. Crowell (06:45:35): > I pushedvignettes/SpatialExperiment2.Rmdjust now, but also dropping the HTML here. Besides restructuring, > * (minor) added linebreaks (2 spaces) in various places for readability, e.g. in bullet points > * added a chunk on sample IDs being protected (> replacement by NULL keeps them) > * rephrased some things to separate explainingspatialCoords/Dataslots in the class vs. arguments in the constructor > * cleaned up thespatialExperiment()constructor section giving 4 examples, which are compared (usingidentical()) to highlight flexibility > …I think that’s it. Happy for any feedback!! Over the weekend or during our next meeting. Few comments from my end: > * note that, restructuring requires using some example data in the beginning, since the constructions comes later. I useexample(read10xVisium)for this. > * (minor thought) I wonder if we can drop the Installation section… I mean, I usually don’t find this in vignettes, only READMEs, because it’s “just another Bioc package” and there are general instructions on the website for how to install packages from release/devel… > * (minor thought) sections 3 & 4 could be switched if we like. structure - construction - operations vs. structure - operations - construction both seem fine to me, and I have no preference. - File (HTML): SpatialExperiment2.html

Dario Righelli (09:32:47) (in thread): > Thanks Helena, I really like the new vignette! > Just some minor comments: > * I agree on removing the installation part, it’s also present in the package Bioc web page > * about the object construction, it’s ok to use theexample(), maybe we can refer to the manual page and/or to the section 3 for construction details > * Also, maybe we can rename section 3.2 and3.3 as“Spot-based data” and “Molecule-based data” (it’s more a question than an observation) > * Can we find something different for section 3.1 title? > * Really, really minor: can we add spaces after commas in the section 2.2.2 in this code?sample_id,image_id = TRUE,TRUE will specify all images; NULL,NULL > * I personally like section 3 and 4 in this order, but I’m open to other thoughts :)

Dario Righelli (09:54:26) (in thread): > I was wondering, because the vignette is pretty long, should we set the sections index as floating? (see SCE vignette)

Helena L. Crowell (09:57:24) (in thread): > Great, thanks for all this Dario! Let’s wait for Lukas’ comments & I can have another round of revisions over the weekend, so we can hopefully do a final discussion on Mo:slightly_smiling_face:

Lukas Weber (16:35:46): > Thanks@Helena L. Crowell, this looks great - I will have another detailed look and think some more about the questions above

2021-07-24

Lukas Weber (11:04:16): > @Helena L. Crowellsorry for delay. If I have a few very minor edits like typos, should I push these as a commit into the pull request? Just checking what works best for you (and so we don’t end up with merge conflicts). Thanks!

Helena L. Crowell (11:10:25): > Sure, PR is best! If there are no other comments we can do a final discussion on Mo

Lukas Weber (12:43:03): > One thing I noticed is the small examples in from the constructors are not identical, so this line: > > all(identical(spe1, spe2), > identical(spe1, spe3), > identical(spe1, spe4), > identical(spe2, spe3), > identical(spe2, spe4), > identical(spe3, spe4)) > > returnsFALSE. > > I think the only differences are that column orders incolDataare flipped - I’ll see if I can figure out a fix (e.g. specifying arguments in a different order?), or otherwise we just explain this.

Lukas Weber (12:46:54): > It is because of the default columnsample_idadded incolDatawhen this is not specified

Lukas Weber (12:54:19): > I think the simplest solution is to remove the above code block, and replace the corresponding text with: > > Importantly, all of the above SpatialExperiment() function calls lead to construction of the exact same object (although column orders in colData may differ, depending on which arguments were provided). > > Otherwise, we would need to specify additional arguments (e.g.sample_id) to make sure all the columns are always in the same order, which makes the examples more complicated, which distracts from the main points here (that we can specifyspatialData/Coordsin various ways). What do you think?

Lukas Weber (13:36:06): > This is great, thanks again@Helena L. Crowell. I have added an additional commit in the pull request, with some minor updates as follows: > * formatting (typos, wording, line breaks, etc - I found out you can only have a single space at the end of lines in list items to wrap correctly) > * fix date format at the start > * added the floating table of contents format > * consistent order of arguments when we list spatialData/Coords, and spelled out the full argument names for clarity (although feel free to change back if you think it is too long / less clear this way) > * removed
from the Space Ranger lines in file hierarchy > * removed the code block I mentioned above since it returns FALSE, and mentioned in text that column order in colData can vary > * updated the subsetting example to use the 10x object, since the other one did not have the columns we were subsetting on > I also agree with removing the Installation part, as Dario suggested. > > I hope this makes all makes sense - will go through the rest of the pull request and paper edits now, so we can discuss everything on Monday.

Lukas Weber (14:39:52): > I added a small unit test for thecolData > spatialData > spatialCoordshierarchy

Lukas Weber (14:46:36): > minor update in docs too (sample01notsample1for default value)

Lukas Weber (14:54:42): > I approved the pull request on GitHub. I think it looks great - thanks again@Helena L. Crowell. I would suggest you go through the additional small edits for the vignette suggested by Dario above (e.g. removing Installation section), and check that you agree with the edits in my additional commits, and then we can discuss on Monday and merge it into Bioc-devel:tada:

Lukas Weber (14:58:12): > Next steps will be: > * update objects inSTexampleDataandTENxVisiumDatapackages to use the newint_colDatastructure (with the separate versions for release/devel in ExperimentHub as we discussed a couple of days ago, so we don’t break release) > * merge paper comments/suggestions

Lukas Weber (15:22:08) (in thread): > I also like the suggestion to add the schematic figure in the vignette. Pasting the latest one from Dario here for reference:

Lukas Weber (15:22:15) (in thread): - File (PNG): SpatialExperimentSplitted0.2.png

2021-07-25

Helena L. Crowell (04:08:24): > Thanks for all this@Lukas Weber. I pulled the updates & have some comments: > * I of course checked this before, and getTRUEfor theidentical()call. I am running Bioc 314 & installed thespatialDatabranch prior to knitting… So, I would suggest we figure out why you getFALSE& ideally keep this chunk because I believe it’ll make it much clearer how the different constructions are equivalent. > * I don’t understand what you mean by “I found out you can only have a single space at the end of lines in list items to wrap correctly”. Putting 2 spaces at the end of an item will force a line break, but the itemisation isn’t affected, no? I added these purposefully to make the lists look nicer & more readable. > * Thanks for noticing the subsetting bug -spe10xis a good idea! > * Thanks for the unit tests & doc fix!!! > * Since we all agree, let’s remove the installation section! > * Vary minor, I know… but how about having section 1 titled “Class structure” or “Class anatomy”? The current title, “The SpatialExperiment class” is quite similar to the vignette title, and I am in favour of catchy/short main headers (Class structure/anatomy, Object construction, Common operations sounds really nice to me). Could also make spatialData & SpatialCoords -> SpatialCoords & -Data. Thoughts?

Lukas Weber (11:43:19): > Ok, thanks! > * Ah ok, for theidentical()call I believe I was using thespatialDatabranch installed within a release version of Bioc 3.13. So this is probably my mistake then from using the wrong version. I’ll check it in 3.14 and add a commit to put back youridentical()code block. > * Re the line breaks - ok no worries, I think this is just a stylistic thing, so I don’t really have strong views. I think it looks slightly nicer when the lines wrap across the full width, which matches everything else that is right-justified. So I had thought this was an oversight, and changed it while I was going through. If it was a deliberate choice and you prefer the custom line-breaks then I am happy to revert these in another commit too. > * Yes I think “Class structure” for the first section is good (I think “anatomy” sounds a little odd but also fine if you prefer this)

Lukas Weber (11:43:54): > I’ll pull any updates that are there now, and add these 2 commits now (identical()and line breaks)

Lukas Weber (12:05:26): > Ok I have added these commits now (and feel free to change any other stylistic things that you don’t agree with - my main argument re stylistic things is always that everything should just be internally consistent). I also changed the word “anatomy” to “structure” in one place. If you pull the commits then I would suggest you remove the Installation section and change the section heading names as mentioned above.

Lukas Weber (12:06:42): > Theidentical()example now gives TRUE for me even withspatialDatabranch installed in Bioc release 3.13. Not sure what was going on there for me yesterday.

Helena L. Crowell (12:09:40): > Thx for all that! I’ll have a look before the meeting tomorrow. But think we’re very close & I’m quite happy with how the vignette looks now:sparkles:

Lukas Weber (12:10:01): > Yes, I think it looks great - thanks for your efforts on this!!

Lukas Weber (12:11:29): > Also note I opened a separate pull request to update the GitHub readme page, which was still referring to earlier versions - we can discuss briefly tomorrow too. I added some info in the pull request.

2021-07-26

Helena L. Crowell (03:09:35): > Okay, had a look and all looks good to me. I did a small PR to > * remove the installation section > * change header “The SPE class” to “Class structure” > * change header “spatialData & spatialCoords” to “spatialData & -Coords” (open for discussion!) > …see you later! I might have to join via phone again, but can easily pull up GH & the paper regardless.

Dario Righelli (03:42:42): > Thanks guys, I just read all of this:slightly_smiling_face:

Dario Righelli (03:44:13): > Why the GHA are failing? it says that the vignette spatialexperiment2.rmd cannot find the spe object, why do we have a second vignette? is it just for backup?

Helena L. Crowell (03:45:19): > it’s just for backup. Since I reorganised the whole thing I wanted to keep the original to make it easier to compare (the HTMLS, as GH compare gets messy)… We can delete it now if we’re all okay with that

Dario Righelli (03:46:11): > sure, thanks!

Dario Righelli (03:46:50): > About the figure, I have the illustrator version of it, I can easily re-organize it in an horizontal way…

Dario Righelli (03:47:21): > Then later we can discuss about the additional “components” to add and how

Helena L. Crowell (03:47:33): > I think that’d be great, and Davide agreed I think? Maybe let’s see what Lukas says before you put a lot of work into it. And I also agree to keep it simple (black/white) and only use colors for highlighting.

Dario Righelli (03:48:33): > I’ll just re-organize in a more horizontal way now, just to have an idea on how it can become. Is that ok?

Helena L. Crowell (08:01:02): > Yes, sounds great, thank you! A sketch version is fine, needn’t be all perfect:wink:

Helena L. Crowell (08:01:22): > I think the OSCA one for SCE could serve as a model

Lukas Weber (08:29:45): > Ok, so same as before but horizontal, yes I think that is fine. I like the colours though - why do we need black and white?

Stephanie Hicks (08:49:34): > sorry everyone, but i’m running 5-10 mins late this morning. please do start without me tho. I’ll join as soon as I can

2021-07-27

Helena L. Crowell (07:58:29): > FYI: I merged thespatialDatabranch intomaster(mainlyspatialData-restructuring & vignette revision) - GHA checks all passed successfully - & pushed to Bioc devel (v1.3.2).

Dario Righelli (08:01:23): > Thanks@Helena L. Crowell!!!:smile:

Lukas Weber (08:45:08): > Awesome, thanks!:tada:

Stephanie Hicks (09:31:28): > thank you!!

Lukas Weber (17:24:32): > I’ll merge the updated GitHub readme from my other small pull request too. Thanks for approving@Dario Righelli. This also has another version bump and updated date in DESCRIPTION.

Lukas Weber (17:25:42): > Oh we should add an updated NEWS too.@Helena L. Crowelldo you happen to have a good 1-line summary off the top of your head?

2021-07-28

Helena L. Crowell (02:29:11): > Very simple, but maybe something like … That’s really it I think. > * spatialData moved from colData to int_colData > * restructuring of vignette & added imgData section

Helena L. Crowell (02:30:49): > Apologies for forgetting about the README & that we need another version bump for this:confused:I suggest, though, that we wait until the current push goes through so we can fix things in case there’s a check/build error, ok?

Lukas Weber (08:13:00): > sounds good - we can add the NEWS in the readme branch too

Lukas Weber (14:55:02): > I added a commit with the NEWS in thereadmebranch in the open pull request > > the build report still shows version 1.3.1, so we can check again tomorrow to see if 1.3.2 has gone through correctly, and if yes then merge thereadmebranch and pull request > > I think it is fine to have an extra version bump, so wouldn’t worry about that

Lukas Weber (15:09:10): > @Helena L. CrowellI have rebuilt some of my STexampleData objects, and I think we may be able to have a single version only: > * SPE object built in old version (1.2.1) (which is currently on ExperimentHub) fails validity check in new version (1.3.2) so needs updating. However: > * SPE object built in new version (1.3.2) passes validity checks in old version (1.2.1, i.e. current release version), so we may not need to keep the old versions > So if my understanding is correct, we can overwrite the objects with the new version, without needing to keep both versions. Is this correct? (Or am I thinking about the validity checks incorrectly? There is no difference from the user perspective, right?)

Lukas Weber (15:09:40): > oh wait

Lukas Weber (15:09:56): > no that is all wrong - it passes the validity check, but can’t accessspatialDatawith the accessor (because the old version of the accessor can’t find it inint_colData)

Lukas Weber (15:10:39): > yeah it doesn’t work - I was only looking at the validity checks, not the accessors. So we do need both versions.

Lukas Weber (15:17:04): > I will prepare the objects today, and wait until the build report for Bioc-devel with SPE version 1.3.2 has gone through (hopefully tomorrow) before contacting the Bioc team to upload the 2 versions (just in case there is an unexpected problem in the build report)

Lukas Weber (15:57:05) (in thread): > Hmm ok after looking at this in some more detail, I don’t think the 2 versions idea will necessarily work. > > From reading the ExperimentHub documentation, the 2 versions would be required to have different ExperimentHub IDs and names, which we don’t want. It doesn’t look like we can just have 2 versions of the same objects. > > Maybe the simplest solution might be to update the objects using the current versions, which passes validity checks in both Bioc-devel and Bioc-release but then can’t accessspatialDatawith the accessor in release. We will be using Bioc-devel in the workshop, so this is fine for using the objects there. If we really need to access the objects in release, we could just useint_colData(spe)$spatialDatato get thespatialData(but we are mainly working in Bioc-devel anyway, so this may not come up at all). What do you think?

Lukas Weber (22:06:42): > I made some updates to ggspavis too.@Helena L. CrowellI did end up getting stuck inplotVisium()- if you do have time to have a look at this one this would be super helpful, thanks. (I think you have push access on GitHub, so if you do get around to it feel free to push any updates - I have pushed all my local updates for today)

Lukas Weber (22:10:05) (in thread): > Latest SPE objects for STexampleData for testing with the Bioc-devel version of SPE are here (to be uploaded to ExperimentHub after SPE devel build report passes), in case you need them for this or any other testing: - File (Binary): Visium_humanDLPFC.rds - File (Binary): Visium_mouseCoronal.rds - File (Binary): seqFISH_mouseEmbryo.rds

2021-07-29

Helena L. Crowell (01:49:38) (in thread): > Had a look just now. What exactly is the problem, could you please clarify? > > The examples work fine for me, although > * sub <- spe[, spatialData(spe)$in_tissue]needs to besub <- spe[, as.logical(spatialData(spe)$in_tissue)] > * not sure how this worked before, but in this linehttps://github.com/lmweber/ggspavis/blob/bbea55eb507029181a5b9a4ff92a45cf95c5f987/R/plotVisium.R#L181,imgisn’t defined, so needed to addimg <- img_df$data[[1]]in the line above > * generally, the issue with faceting multiple images and usingcoord_fixedis that this assumes each image has the same size (nrow/ncolof therasterobject). I haven’t really figured out how one would facet with varying image sizes.

Helena L. Crowell (03:58:02) (in thread): > Hm. My plan was actually > * keep release as it is. Not object updates. Not subdirectories. > * make a v1.1.1 subdir on devel with new objects > * the metadata.csv paths need to be changed accordingly! > * make Lori update objects on devel using the new subdir structure > * release shouldn’t be affected. And with the next release (1.2.0), the 1.1.1 directory still exists on the AWS, and any new updates would go into 1.2.1 on release / 1.3.1 on devel etc.

Lukas Weber (10:59:02) (in thread): > Ok thanks. This was my original plan too - from my reading yesterday I thought this didn’t work. I’ll check again - I agree if this works then this would be best

Lukas Weber (10:59:23) (in thread): > Ok thanks - will make these updates and do some more testing today

Lukas Weber (12:32:47) (in thread): > Fixed the reversed y coordinates thing that I was previously confused by too (to match the spots onto the histology image) - it needed to bey_tmp <- nrow(img) - y_tmpinstead ofy_tmp <- max(y_tmp) + min(y_tmp) - y_tmp:tada:

Lukas Weber (13:43:07): > We got an error in the build report in the checks. I will have a look at it now:http://bioconductor.org/checkResults/3.14/bioc-LATEST/SpatialExperiment/nebbiolo2-checksrc.html

Lukas Weber (13:51:59): > Ah it is this thing, which I remember others on Bioc have talked about. I don’t know why this is causing an error for us: > > Error in (function (cache = getBFCOption("CACHE"), ask = interactive()) : > DEFUNCT: As of BiocFileCache (>1.15.1), default caching location has changed. > Problematic cache: /Users/biocbuild/Library/Caches/BiocFileCache > See[https://www.bioconductor.org/packages/devel/bioc/vignettes/BiocFileCache/inst/doc/BiocFileCache.html#default-caching-location-update](https://www.bioconductor.org/packages/devel/bioc/vignettes/BiocFileCache/inst/doc/BiocFileCache.html#default-caching-location-update) >

Lukas Weber (14:08:26): > Hmm ok so looks like this is the part that doesn’t work. I also confirmed it doesn’t work in a devel version on our cluster just now: > > #' # add an image > #' url <- "[https://i.redd.it/3pw5uah7xo041.jpg](https://i.redd.it/3pw5uah7xo041.jpg)" > #' spe <- addImg(spe, > #' sample_id = "section1", > #' image_id = "pomeranian", > #' imageSource = url, > #' scaleFactor = NA_real_, > #' load = FALSE) >

Helena L. Crowell (14:15:02): > Hm. And what solution do they propose? Or is it an issue that’ll resolve itself on the Bioc end?

Lukas Weber (14:15:42): > I’m not sure. I was just trying it out on our cluster, and I get the same error when doing the following: > > > spe <- addImg(spe, sample_id = "section1", image_id = "pomeranian", imageSource = url, scaleFactor = NA_real_, load = FALSE) > > imgData(spe) >

Lukas Weber (14:15:57): > So it seems not only on the Bioc builders. Strange that I didn’t see the error previously.

Lukas Weber (14:16:32): > Or I wonder if I delete my local cache on the cluster - maybe this is something left over from previously

Lukas Weber (14:18:52): > yeah now it works

Lukas Weber (14:19:28): > So we need to ask them to delete our old cache in the builders - some more details here:https://www.bioconductor.org/packages/devel/bioc/vignettes/BiocFileCache/inst/doc/BiocFileCache.html#default-caching-location-update

Lukas Weber (14:19:34): > I’ll write an email to the Bioc team

Lukas Weber (14:32:23): > Sent to Bioc-devel (cc’d@Helena L. Crowell@Dario Righelli)

Lukas Weber (14:33:28): > Response already from Lori: > > Kern, Lori > 2:32 PM (0 minutes ago) > to me, bioc-devel@r-project.org, Helena > > Thank you for making us aware. > > Its interesting because I have already cleaned this up and initialized the caches on the builders so I suspect there is a package that is somehow force creating the old location. > > Regardless, I will take care of correcting on the builders and notifying any offending package. > > Cheers, >

Lukas Weber (14:37:21): > The rest of the build report is okay (installing and building), so I would suggest we go ahead and merge the other pull request with the readme and NEWS so it is all done.@Helena L. Crowelldo you agree or prefer to wait?

Helena L. Crowell (14:46:34): > Fine with me to move forward! But not online anymore now, I’m at dinner:shallow_pan_of_food:

Lukas Weber (14:54:02): > Ok, I’ll go ahead and push it to devel, and will keep monitoring the build reports. Enjoy your dinner!!:shallow_pan_of_food:

Lukas Weber (15:01:54): > Do we still need this open issue? Otherwise will close this too.https://github.com/drighelli/SpatialExperiment/issues/25

2021-07-30

Helena L. Crowell (01:08:08) (in thread): > Technically it’s not resolved… but very specific to Visium. And the diameter is fixed until HD comes out, where it’ll be fixed again so… not sure we really need this.:face_with_monocle:

Helena L. Crowell (02:40:06) (in thread): > Got this from Kayla in response to an inquiry I made:Thanks for the inquiry. Please see responses below: > 1. The ordering of availability and putting objects in the hub indeed can be a little tricky. If you prefer to make sure the vignette doesn’t fail, you could send the metadata.csv file in an email, or if you have a github repository for the package, point us to a branch that has the required information. > > 1. Correct that it won’t matter from the interface side as the S3 bucket and the RDataPaths in the database will point to 1.1.1. An alternative would be to use the BioC release as a version instead of package version, thus indicating which release it was first available. Although similarly will show a previous version like 1.1.1. Not entirely helpful answer. In the end you can choose the sub-directory name however you like so if you think a different distinction would be more clear you are welcome to call the sub-directory anything you like.

Dario Righelli (08:29:33): > So we still have problems to access the devel version of the class through bioconductor, but this time is not our fault!:smile:

Dario Righelli (08:35:04): > @Lukas Weberand@Helena L. Crowellwhen you have some time, could you please update your vignettes in the workshop? If you prefer I can try to fix them… > Also, if we fix the figure for the paper, I can update the figure in the introduction part

Lukas Weber (08:35:41) (in thread): > Ah yes will do - maybe this afternoon

Lukas Weber (14:21:11) (in thread): > Thanks - I closed the issue now (with a comment that this may come up again in the longer-term future)

Lukas Weber (14:22:39) (in thread): > Ok, thanks for this

Lukas Weber (14:23:58): > Build report didn’t update today (I think yesterday’s update was after the cutoff time), so will check again tomorrow

2021-07-31

Lukas Weber (13:46:04): > Build report still has the same error - but I think this will resolve once the Bioc team has identified the package that is messing up the cache on the builders. Will keep monitoring it daily.

Lukas Weber (13:47:01): > I have also pushed an update to the SpatialExperiment workshop for my part (chapter on loading dataset from STexampleData and generating some plots)

2021-08-02

Dario Righelli (10:58:02): > Do you know if Lori already worked on this?

Helena L. Crowell (12:30:22): > Re EH: Kayla is in charge. And yes, we are in active communication. > Re cache issue: Yes, I believe they are working on it.

2021-08-03

Lukas Weber (14:45:52): > Two updates: > * SpatialExperimenthas now passed in the build report with version 1.3.3, so the caching issue is resolved > * Workshop has built correctly using all the GitHub versions. I pushed an additional change to removeTENxVisiumDatafromImportswhich fixed it

Lukas Weber (14:49:24): > Kayla has also added the updated EHub objects forSTexampleDataSo this means we could now update the workshop to use Bioc-devel forSpatialExperimentif we prefer. However we would still need to leaveSTexampleDatafrom GitHub until it goes through the build report to the latest version 1.1.7 in Bioc-devel.

Lukas Weber (15:09:20): > Actually looks like the build report shows version 1.3.3 of SPE has passed, but the devel landing page is still showing version 1.3.1 (old version), so it may take another day or so to propagate. So best to leave the workshop with the GitHub version for now.

Lukas Weber (15:13:39): > Rendered version of workshop pages are here for checking:https://drighelli.github.io/SpatialExperiment_Bioc2021/index.html - Attachment (drighelli.github.io): SpatialExperiment_Bioc2021 > Cells spatial organization within tissues is important for the understanding of cell communications and interactions in developmental, physiological, and pathological states. Lately several technologies are emerging for the joint extraction of transcripts quantification and their spatial organization at single cell level. Among others, seqFISH and 10x Visium Spatial Gene Expression (VSGE) seem to be the most widely used platforms because of their ability to extract hundreds to thousands of transcripts at the same time in the first case and for the standard protocol implemented in the latter. Briefly, seqFISH combines temporal barcodes in multiple hybridization rounds with microscopy fluorescent imaging to detect the spatial coordinates of transcripts. On the other hand, VSGE allows the joint analysis of microscopy images of a chip-placed frozen piece of tissue and its transcriptome at spot-barcode level. Given the lack of dedicated general tools for spatial transcriptomics, we developed the SpatialExperiment Bioconductor package, which presents a set of two dedicated classes (SpatialExperiment and VisiumExperiment) both extending the SingleCellExperiment Bioconductor class with slots and methods for spatial transcriptomics data handling. This work could become the basis for facilitating further development of methods aimed to analyze this type of data, e.g., for spatial clustering and the identification of spatially variable genes. Here, we present a short workshop showing how to analyze spatial transcriptomics with our SpatialExperiment package and how to use it in combination with other analysis software. We will start by showing how to combine a SpatialExperiment with a SingleCellExperiment to store multimodal datasets such as seqFISH and scRNAseq and how to manage them to graphically visualize the data. Additionally, we will show how to create a VisiumExperiment object starting from 10x Genomics public datasets and how to manage it for clustering annotation and plotting.

Helena L. Crowell (15:24:17): > I am too tired to think straight… but what’s our call here? I also wasn’t fully aware it’s tomorrow already… I am fine either way. Just need to do a test run / live code practice tomorrow, but I think I’m pretty prepared by now to demo this…

Helena L. Crowell (15:24:58): > But definitely need to know soon. Because the different version have different structures, since I do the class demo :/

Lukas Weber (15:25:10): > Yeah I think we leave it as GitHub versions, which is working now (after my last push to remove TENxVisiumData from Imports)

Lukas Weber (15:25:21): > It is passing:https://github.com/drighelli/SpatialExperiment_Bioc2021/actions

Lukas Weber (15:25:39): > And this is using version 1.3.3 (latest version) of SpatialExperiment

Helena L. Crowell (15:27:13): > Great!! Yeah, I saw the eh issue and emailed Kayle. She said they keep older versions for reproducibility. But, I get errors when trying to install so I didn’t really get her response :/. The correct objects are on the aws, but everything is duplicated:face_with_hand_over_mouth:

Helena L. Crowell (15:27:34): > So I’ll practice the demo using devel tomorrow, correct?

Lukas Weber (15:27:44): > Yes, there are now two sets of EHub IDs for the objects (old and new)

Lukas Weber (15:27:51): > Yes, we will use devel version (1.3.3) tomorrow

Helena L. Crowell (15:28:26): > Ok. I’m going to bed now:yawning_face:…and maybe take a nap before our session:sweat_smile:

Lukas Weber (15:29:11): > One way to fix the ambiguity in having two sets of EHub IDs could be to give the old ones a slightly different name, e.g.Visium_humanDLPFC_3_13(with the3_13version number) - but we can update this later in themetadata.csvif we want to

Lukas Weber (15:29:42): > orVisium_humanDLPFC_deprecated

Helena L. Crowell (15:31:06): > Yeah I guess… I just thought having separate subdirs takes care of this. Wasn’t aware separate paths would keep identical IDs. I think I’ll eventually email Kayla again and see what she recommends as this can’t be the first time this happens (I hope ;)

Lukas Weber (21:39:42): > There were some issues with build errors from dependency packages in Bioc-devel (specifically DelayedMatrixStats), which I think makes sense since devel isn’t really meant to be fully stable. So I updated the workshop repo to use Bioc-release plus the latest GitHub versions of SPE / STexampleData / ggspavis. It is passing again now, so is ready for tomorrow. I might do another small cleanup in my sections but I think looks good overall, and this is using the latest SPE structure.

Lukas Weber (22:01:51): > Ok I pushed a few more minor updates and cleanups. I updated DESCRIPTION since it was still referring to VisiumExperiment, and also updated the version number. I think this looks good now, and is all passing for tomorrow.

2021-08-04

Dario Righelli (03:23:58) (in thread): > I agree

Dario Righelli (03:28:03): > Thanks guys, I just read all the messages!

Lukas Weber (08:53:16): > Yep I think it’s all good now. I made some edits last night and checked the docker image which has the “Bioc-release + GitHub” combination of packages. It works on my Mac laptop, 1.5GB download.

Dario Righelli (08:56:23): > Only thing is that our workshop is not in the orchestra list

Dario Righelli (08:56:40) (in thread): > I saw, thanks for all the edits!

Lukas Weber (08:57:11): > Ah yes - I noticed that too. You mean this list? Maybe worth emailing to add it:https://bioc2021.bioconductor.org/workshops/

Lukas Weber (08:57:59) (in thread): > (Using Bioc-devel didn’t work because then it also installs the whole stack of dependencies from Bioc-devel, some of which have build errors. So using Bioc-release plus GitHub for our latest updated packages was the best approach.)

Lukas Weber (09:03:22): > Oh you also mean the separate Orchestra platform - I don’t know about that one

Dario Righelli (09:21:55) (in thread): > I think this one is for long workshops only…

Dario Righelli (09:22:03) (in thread): > yes this one

Lukas Weber (09:57:20) (in thread): > :+1:

Lukas Weber (12:07:46) (in thread): > Orchestra is only for long workshops - Levi Waldron just mentioned now

Lukas Weber (12:08:32) (in thread): > It includes free cloud instances of RStudio. So we do not have this, but we still have the docker images and instructions for local installation

Lukas Weber (20:50:01): > I made a couple of minor edits in the workshop repo / docker build (updated GitHub installation instructions; changed “Cell Ranger” to “Space Ranger” in text) so that we have a final version there for posterity. Right now it looks like the docker build is broken again though, so will make sure I figure it out and get a final working version again.

Lukas Weber (23:25:31): > There were some changes pushed to dependency packagesMatrixGenericsandDelayedMatrixStatstoday which broke them in release and devel, and now everyone’s workshops are failing:upside_down_face:https://github.com/PeteHaitch/DelayedMatrixStats/issues/84

Lukas Weber (23:25:51): > Will monitor this and make sure we have a final working Docker image in the end so people can use it in the future

Lukas Weber (23:26:21): > It only seems to affect the Docker images

2021-08-05

Lukas Weber (10:13:11): > Ok it worked now with the fixes forDelayedMatrixStatsrecommended by Pete (putting it inRemoteswith a specific commit hash). So we have a working version of the Docker image now, which can be used in the future if we need it:tada:

Lukas Weber (10:14:20): > He has also updatedDelayedMatrixStatsin release and devel so the issue will go away in a few days once it has gone through the builds. So we don’t need to worry about it in the future e.g. forggspavissubmission.

Hirak (15:18:22): > @Hirak has joined the channel

2021-08-08

Lukas Weber (15:38:54): > Submittedggspavisto Bioc:https://github.com/Bioconductor/Contributions/issues/2220

Lukas Weber (21:26:11): > Reminder we have our weekly / fortnightly meeting tomorrow (for those who are not on holidays)

2021-08-09

Helena L. Crowell (01:07:05) (in thread): > Thanks for the reminder, Lukas! Just to clarify: We discussed putting meetings on hold / meeting less frequently after BioC21 & paper submission. Now, I believe there are only few comments left from Leo & Shila, with responses from the other co-authors still to come. Would it be worth waiting for these before we meet again? Not sure if there’s anything else on the table than finalising the MS at the moment?

Lukas Weber (07:51:37) (in thread): > Yes, I think we probably only need one or two more meetings, unless we have future issues regarding the SPE structure. This week we can wait for additional comments from co-authors (I think I asked for them by mid week), and then submit the paper. We can either have the meeting today or skip it if you are busy, either works for me.

Helena L. Crowell (07:53:18) (in thread): > Im fine either way. Just not sure what we’d discuss ;)

Lukas Weber (07:54:49) (in thread): > Maybe just confirming paper submission plans with@Davide Rissoif he is available

Lukas Weber (08:01:38) (in thread): > There was a question of whether to update the bioRxiv preprint - I strongly prefer updating it at the time of submission

Lukas Weber (08:31:16) (in thread): > I just realized we didn’t actually have a meeting scheduled today. It isn’t in my google calendar:joy:

Lukas Weber (08:32:04) (in thread): > Ok then unless anyone has anything more to add, I think we can just say this week we will try to submit the paper after receiving comments from everyone?

Stephanie Hicks (08:35:54) (in thread): > Hey just seeing this. Yeah I don’t have a meeting in calendar

Stephanie Hicks (08:36:21) (in thread): > I’m doing daycare drop off right now so I can’t join this morning.

Lukas Weber (08:36:24): > Yep no meeting today, I mixed up the weeks (see thread above)

Lukas Weber (08:36:36) (in thread): > No worries, yes I mixed up the calendar, no meeting today

Stephanie Hicks (08:36:55) (in thread): > But let me know what I can do to support helping getting the paper resubmitted

Lukas Weber (08:37:22): > This week the plan will be to merge any additional comments in the paper from co-authors (I asked for them by mid-week), and then resubmit the paper

Stephanie Hicks (08:37:40): > Sounds good. Thanks everyone!

2021-08-16

Helena L. Crowell (08:15:29): > Meeting, yes?

Lukas Weber (08:21:55) (in thread): > yep

Lukas Weber (09:32:17): > Ok finished merging comments - I’ll post the links in DM

2021-08-24

Lukas Weber (15:48:00): > @Helena L. Crowellwould you be willing to put the SpatialExperiment schematic into the public domain? I think this is a very nice figure and it would be good if we can re-use it whenever we want in the future, e.g. on websites or in other papers. But some journals have quite restrictive / sneaky policies about reusing your own figures if they have already been published. So one way around this is to put the image into the public domain before publishing it. > > So in this case we could e.g. upload it to Wikimedia Commons and select a permissive “CC-BY” license (same license as on most of our bioRxiv papers, which lets anyone use it for any purpose as long as they acknowledge the creator), or even the “CC0” (no attribution necessary; although I would probably go for CC-BY). Then it is in the public domain until the end of time and we can use it for whatever purposes we want in the future. > > Technically it is probably already in the public domain under CC-BY license since we put it in the bioRxiv paper. But doing it explicitly via Wikimedia Commons would remove any possible future ambiguity.

Helena L. Crowell (15:49:34): > Sure- so how do I do that? Shall I or do you have experience with this? Is dpi 300 sifficient? If so, this is on here I think. Else I can export a HD version

Lukas Weber (15:49:41): > Unless we re-used parts of an existing figure within it - then we need to be careful that we are not going against a previous license

Helena L. Crowell (15:50:00): > I think it’s quite distinct from the s(c)e ones… but not sure how to define “reused”

Lukas Weber (15:50:13): > Yeah if you made it from scratch then I think it is just a quick upload on Wikimedia Commons:https://commons.wikimedia.org/wiki/Main_Page - Attachment (commons.wikimedia.org): Wikimedia Commons

Lukas Weber (15:50:26): > I think there is an upload link there somewhere

Lukas Weber (15:50:32): > https://commons.wikimedia.org/wiki/Commons:Upload#/media/File:MediaWiki-Upload-form.png

Helena L. Crowell (15:50:34): > Ok, happy to do this tomorrow! Thx!

Lukas Weber (15:50:38): > Great, thanks!

Helena L. Crowell (15:51:29): > It’s from scratch in inkscape. But sure, the boxes and rows and cols are similar… I think we should be good though as nothing is pasted or looks the same?

Lukas Weber (15:51:45): > I think Wikimedia Commons also includes anything that is on Wikipedia - and they also have a separate upload link for direct uploads

Lukas Weber (15:52:05): > Yep as long as it isn’t directly edited from an existing image it should be fine

Lukas Weber (15:53:07): > Thanks!

2021-08-25

Helena L. Crowell (02:32:08): > Done.https://commons.wikimedia.org/wiki/File:SpatialExperiment.png - Attachment (commons.wikimedia.org): File:SpatialExperiment.png - Wikimedia Commons

Lukas Weber (08:15:46): > Awesome, thanks

2021-09-01

Lukas Weber (14:27:19): > ggspavisis accepted into Bioc now:tada:https://github.com/Bioconductor/Contributions/issues/2220

2021-09-06

Eddie (08:23:41): > @Eddie has joined the channel

Eddie (09:25:45): > @Eddie has left the channel

2021-09-14

Lukas Weber (09:49:56): > Just putting a link to the new issues Aaron Lun raised here so we can discuss:https://github.com/drighelli/SpatialExperiment/issues

Lukas Weber (09:50:58): > These are good points, so we should address / discuss. I think the conversion function from SPE to SCE will need some thought (e.g. do we just drop the non-relevant parts?), and the others are simpler

2021-09-15

Lukas Weber (12:07:59): > @Dario Righelli@Helena L. CrowellI was looking at the various branches on GitHub and Bioconductor. It looks like Bioconductor /upstreamis now a few commits ahead of GitHub /origindue to a couple of edits from Herve Pages and Marcel Ramos. Just checking if you are okay with me pushing these toorigintoo so we don’t end up with merge conflicts for the new issues. Thanks!

Dario Righelli (12:12:34): > Oh right, I think it’s really important to update the devel branch on the bioc because of the updates in the SE class. > Thanks for noticing Lukas!

Lukas Weber (12:13:33): > Ok cool - yes I’ll update them and push them all now so it is all consistent, since currently Bioc-devel / upstream is ahead of GitHub / origin

Lukas Weber (12:14:31): > I also noticed we may have a bug inread10xVisium()- there is a part of the pathouts/missing in the defaults. I’m checking now to see if it is really a bug or if I am misunderstanding something. Aaron also had some comments here:https://github.com/drighelli/SpatialExperiment/issues/76

Lukas Weber (12:15:00): > @Helena L. Crowellmight know more about this one too

Lukas Weber (14:23:29): > I opened a pull request for this one. Could you please both have a look at this@Dario Righelli@Helena L. Crowell, since this is a slightly larger change. Aaron pointed out that it is technically a breaking change, but I think this is okay since we are working in Bioc-devel, and I have also included a NEWS item. We can also discuss in our next meeting. Thanks

2021-10-04

Dario Righelli (03:56:46): > Hi guys, have you seen the new issue on the SpatialExperiment?https://github.com/drighelli/SpatialExperiment/issues/82

Lukas Weber (15:17:12): > Thanks - just saw now. I think the answer is that spatialcoordinatesare inspatialCoords(), and everything else spatial is inspatialData()? Maybe we need to make this more clear in some way.

Lukas Weber (15:38:34): > I added some comments

2021-10-14

Lukas Weber (12:14:39): > Maybe the most substantive comment from the reviews is the same as the one above – i.e. clarifying the distinction betweencolDataandspatialData, and asking whether we really needspatialData

Lukas Weber (12:15:26): > I thinkspatialCoordsis easier to explain, since that is simply the coordinates

Lukas Weber (12:30:06): > After re-reading the comments, I think everything else is very addressable, mainly with additional examples / vignettes / text. But the question of whether we really needspatialDataand/or how to explain its purpose/usage better is a good point to discuss for tomorrow.

Lukas Weber (12:34:43): > A drastic option could be to change the getters so thatcolData()can access everything inspatialData(and/or vice versa) so they are basically the same thing. But then what is the point of havingspatialDataat all. However the same argument could be made for the existingreducedDimslot – i.e. why is all that not also incolData. So maybe it is more a matter of explaining it better.

Lukas Weber (13:02:05): > Another drastic option could be to deprecatespatialDatabut still have thespatialData()getter/setter letting people access/store things incolDataso people’s code doesn’t break for backward compatibility

Lukas Weber (13:02:59): > ForspatialCoordsI think everything is fine since we have a strong use case / example – it is stored as a numeric matrix which allows passing to other functions easily

Lukas Weber (13:03:16): > So forspatialDataI guess we need some clear use case / examples to reply to the reviewers

Lukas Weber (13:03:53): > And/or comparing toreducedDim

Dario Righelli (13:04:04): > If we move thespatialDatainto thecolDatawe go back to the “loop” issue that obliged us to move it into theint_colData

Lukas Weber (13:04:12): > Ah yeah

Dario Righelli (13:04:37): > I agree though that we need a strong use case for the reviewer

Dario Righelli (13:05:10): > I’m sure it will come in mind again, because we put lots of thoughts on this…

Dario Righelli (13:07:11): > Anyway if we decide to deprecate it, we need to rethink the entire data structure and to put an object “versioning” (as in the SCE) for facilitating the conversion of the object

Lukas Weber (13:16:49): > I’m very reluctant to deprecate something core like this, since it affects backward compatibility, but I can sort of see their point about it being redundant. But yes agree that in this case we could also think about a simple conversion function (which in this case could simply take everything inspatialDataand move it intocolData).

Lukas Weber (13:21:26): > We could call them versions 1.0 and 1.1 or something like that

Dario Righelli (13:23:47): > yes the problem is still about the loop issue with the actual structure:sweat_smile:

2021-10-15

Lukas Weber (09:20:02): > Quick update for others here from our discussion on the issue above – we do have a clear use case forspatialData, which we can explain much better in the paper. Specifically, thespatialDataslot is for things that are provided from the original experiment (e.g.in_tissue,array_col,array_rowfor Visium), which generally will not be modified any further by the user. By contrast,colDataincludes columns generated and/or modified by the user, such as cluster labels or QC values. So I think this is quite clear, and a good justification for keepingspatialData. Also, havingspatialDatamakes it easier for a user to get a quick overview of the important columns from the original experiment, sincecolDatacan get very large. And finally there is also the precedent fromreducedDims, which is also separate.

2021-10-18

Qirong Lin (19:34:13): > @Qirong Lin has joined the channel

2021-10-25

Lukas Weber (11:47:10): > Follow-up from meeting today – we are planning to move the contents ofspatialDataintocolData, following the feedback from users that having separatespatialData/colDatais confusing. For backward compatibility we will also keep an accessorspatialData()that accesses defined columns ofcolData, but these columns are all stored incolDataand can also be accessed directly with$. ThespatialCoordsslot will continue to be a separate slot (similar toreducedDims) and stored as a numeric matrix for easy passing of spatial coordinates to other packages. So in the end there will becolDataandspatialCoords, but nospatialData(except the accessor). More details in issue here:https://github.com/drighelli/SpatialExperiment/issues/84

2021-10-26

Lukas Weber (22:58:39): > Just moving this here from the other chat. I responded to the GitHub issue with Seurat’sSpatialFeaturePlot()giving an error related toSpatialExperiment. I think it might be due to an older version ofSpatialExperiment, although I’m not completely sure. I installed the Seurat stack and tried one of their examples to reproduce the error from the GitHub issue, and it did not give an error for me:https://github.com/satijalab/seurat/issues/5128

2021-10-27

Dario Righelli (05:28:41) (in thread): > thanks Lukas!

2021-10-28

Helena L. Crowell (09:55:11): > Feedback on this appreciated -https://github.com/drighelli/SpatialExperiment/pull/81#issuecomment-953867947- thanks:pray:

Dario Righelli (10:28:03) (in thread): > done

2021-10-29

Helena L. Crowell (01:23:13) (in thread): > had another round, sorry. if things get out of hand / confusion / we can’t reach an agreement, perhaps we should schedule a meeting. No rush tho, afterall, Bioc 3.14 was just released.

Dario Righelli (04:20:25) (in thread): > I think it’s better to move the conversation in the main channel…

2021-11-24

Lukas Weber (15:03:13): > Thanks Helena for checking PR 87. I have merged it intoorigin/masternow

Lukas Weber (15:04:28): > I also synced a new branchRELEASE_3_14onoriginwhich was previously only onupstream, so we have it in GitHub too in case we need it later. Our current work is only affectingmasterand Bioc-devel though

Lukas Weber (15:05:17): > I leftDESCRIPTIONalone for now (re version numbers) so we don’t get merge conflicts when we merge the other PR. So it is still 1.3.6 for now.

Lukas Weber (15:06:52): > When we have finished / merged PR 73 (the last remaining one from the smaller ones before the restructure) we should maybe also merge in the version number commits from the Bioc team in Bioc-devel /upstream/master, so we don’t end up with merge conflicts inDESCRIPTIONfor the restructure.

Lukas Weber (15:07:38): > We could also think about pushing toupstreamafter merging PR 73 in case there are any unexpected issues before the restructure.

2021-12-14

Megha Lal (08:23:55): > @Megha Lal has left the channel

2022-01-03

Kurt Showmaker (17:05:30): > @Kurt Showmaker has joined the channel

2022-02-03

Lukas Weber (15:56:56): > This issue just came up again in discussions today. I had thought it was only an issue for older versions of Seurat / SpatialExperiment, but seems this may not be the case. Just posting here for now so we can keep track of it, and I will try to figure out what is going on:https://github.com/satijalab/seurat/issues/5128

Lukas Weber (16:20:28): > Ah I think it is in part due to the fact that both Seurat and SpatialExperiment have a class calledSpatialImage. So if users have both packages installed, Seurat is somehow calling the wrongSpatialImage(but only in some cases)

Lukas Weber (16:28:20): > Not sure what the best solution is. Maybe we could put::everywhere but then that only solves it in one direction (i.e.SpatialExperimentwould work correctly, but Seurat still will not. And currently the advice from Seurat users is simply to uninstallSpatialExperimentwhen this happens, so this is not great for us)

Lukas Weber (16:28:34): > We could rename ourSpatialImageclass, but that will be a hassle

Lukas Weber (16:28:55): > I’ll open a github issue so we can think about it some more

2022-03-31

Nicole Ortogero (22:27:58): > @Nicole Ortogero has joined the channel

2022-05-25

Sanket Verma (17:44:20): > @Sanket Verma has joined the channel

2022-09-14

Robert Ivánek (05:19:36): > @Robert Ivánek has joined the channel

2022-10-06

Lukas Weber (14:06:15): > Pasting the issue@Dario Righellimentioned so we can also discuss in more detail here:https://github.com/Bioconductor/Contributions/issues/2753

Lukas Weber (14:07:24): > It sounds like the plan is to move loader functions for both SCE and SPE objects (DropletUtils::read10xCounts()andSpatialExperiment::read10xVisium()) into this package instead. The argument is that this would streamline the distinction between the underlying data structure and loader functions, as mentioned by Aaron in the issue above.

Lukas Weber (14:07:42): > I think this could make sense

Lukas Weber (14:08:18): > It could also be a good opportunity to resolve some outstanding issues we have re inconsistent default column names between SCE and SPE objects, as discussed in this issue:https://github.com/drighelli/SpatialExperiment/issues/100

Lukas Weber (14:08:47): > Any thoughts / comments?

Lukas Weber (16:31:02): > Just saw Marcel’s PR, will have a look at it, thanks!@Dario Righelli@Helena L. Crowell

2023-01-08

Michael Lynch (06:15:41): > @Michael Lynch has joined the channel

2023-01-09

Luca Marconato (11:30:47): > @Luca Marconato has joined the channel

2023-01-26

Chenyue Lu (16:50:01): > @Chenyue Lu has joined the channel

2023-02-10

Dario Righelli (11:05:42): > Dear all (<!here>), we are facing a weird error on windows during the vignette building. > I’m reporting it here because it would be nice to have additional comments and thoughts, or, even better, possible solutions to this. > 1. I’ve already tried a suggestion from@Hervé Pagès, but unfortunately it doesn’t solve it (the mail in the issue thread on githubhere) > 2. I tried to execute our vignette on a windows machine and it runs well till the end. The real problem is about thecheck()process which produces the error. > This issue is pretty problematic and we need to solve it to keep our class in Bioconductor I suppose. - Attachment: #133 Error building vignette in windows > building vignette in windows fails > > check https://bioconductor.org/checkResults/3.17/bioc-LATEST/SpatialExperiment/palomino3-buildsrc.html
> and https://github.com/drighelli/SpatialExperiment/actions/runs/3909606915/jobs/6680868431 > > trying to fix it based on Harve’s suggestion: > > > Note that we observe the same problem on our daily Windows builder, so this has not much to do with your GitHub actions setup: > > [https://bioconductor.org/checkResults/3.17/bioc-LATEST/SpatialExperiment/palomino3-buildsrc.html](https://bioconductor.org/checkResults/3.17/bioc-LATEST/SpatialExperiment/palomino3-buildsrc.html) > > It looks to me that some recent changes to the package broke the dim() method for StoredSpatialImage objects. Here is how the method is defined in BioC devel: > > > selectMethod("dim", "StoredSpatialImage") > Object with tracing code, class "MethodDefinitionWithTrace" > Original definition: > Method Definition: > > function (x) > { > src <- imgSource(x) > src <- normalizePath(src) > src <- paste0(["file://"](file:///), src) > img <- .get_from_cache(src, NULL) > if (!is.null(img)) > return(dim(img)) > img <- image_read(src) > tib <- image_info(img) > c(tib$height, tib$width) > } > <bytecode: 0x55a3f2dee5b8> > <environment: namespace:SpatialExperiment> > > Signatures: > x > target "StoredSpatialImage" > defined "StoredSpatialImage" > > ## (to see the tracing code, look at body(object)) > > I'm not sure I understand the logic for prefixing the already normalized local path with ["file://"](file:///), but this breaks magick::image_read() on Windows. Without the ["file://"](file:///) prefix, the dim() method seems to work fine on all platforms. >

Hervé Pagès (11:05:57): > @Hervé Pagès has joined the channel

Hervé Pagès (17:11:07) (in thread): > As I said I don’t think you need to add thefile://prefix to the normalized file path below: > > setMethod("dim", > "StoredSpatialImage", > function(x) { > src <- imgSource(x) > src <- normalizePath(src) > src <- paste0("file://", src) > img <- .get_from_cache(src, NULL) > if (!is.null(img)) return(dim(img)) > img <- image_read(src) > tib <- image_info(img) > c(tib$height, tib$width) > }) > > srcis the path to a local file andmagick::image_read()is prefectly able to handle that, so there’s nothing to gain by trying to turn this path into a URL. In this case it actually hurts because somehow it seems to breakmagick::image_read()on Windows. Have you tried to remove that? AFAICT the latest devel version ofSpatialExperiment(1.9.3) still has this problem. > Also it looks like the package has not been touched since Nov 2022 so if you really want to try things and see how they go thru our build machines, you need to commit and push whatever changes you made.

2023-02-11

Lukas Weber (19:22:30): > Thanks@Hervé Pagèsfor the suggestion in the thread above. I think this has worked now.@Dario Righelli@Helena L. CrowellI sent a pull request on GitHub with the fix – could you please check and approve. Thank you! > > Also if this fix looks ok then how about we archive both this channel and the other one#spatialexperiment(as suggested by Vince Carey in#spatial), since we are not using these channels very often – we can continue/consolidate the discussion on other issues in the future in#spatialinstead.@Dario Righelli@Helena L. Crowellcould you please give a thumbs up/down emoji below if you agree/disagree. Thank you!

2023-02-14

Sean Davis (12:23:59): > archived the channel