BIOP / qupath-extension-abba

QuPath extension for Aligning Big Brains and Atlases
Apache License 2.0
6 stars 2 forks source link

add option to overwrite a previous atlas import #19

Closed carlocastoldi closed 6 months ago

carlocastoldi commented 7 months ago

With this PR:

carlocastoldi commented 7 months ago

a couple of additional questions:

NicoKiaru commented 7 months ago

a couple of additional questions:

* is there a particular reason to why `loadWarpedAtlasAnnotations()` does not return the Root annotation?

Not really, I think it could return the root annotation without any problem.

* should we fix the tests so that they're actually useful?

Sure, if you know how to do that, that'd be nice.

* do you think we should make `LoadAtlasRoisToQuPathCommand` be a little be more complex?

Hmm, why not. But this one is simple, and people like simplicity. Why not adding another one if you need more fine tuning. But more importantly, what do you have in mind in terms of complexity?

Thanks for the PR!

NicoKiaru commented 7 months ago

All good, thanks!

NicoKiaru commented 7 months ago

So, here's the commit message:

Should I merge like that or do you plan to add other stuff in this PR ?

carlocastoldi commented 7 months ago

actually, when was the last time that the script with all the repeated defs worked? In the most recent version it gives a syntax error.

Do you still want to put a non-compilable script just so that people have to read and copy the parts that are interesting to them? I feel like this shouldn't be the work of a script shipped with the extension. For that the website/documentation is perhaps the better place?

ERROR: startup failed:
QuPathScript: 65: The current scope already contains a variable of the name myObjectsWithinAList
 @ line 64, column 5.
   def myObjectsWithinAList = allRegions
       ^

QuPathScript: 81: The current scope already contains a variable of the name myObjectsWithinAList
 @ line 81, column 5.
   def myObjectsWithinAList = getAnnotationObjects()
       ^

QuPathScript: 85: The current scope already contains a variable of the name objectsOtherThan
 @ line 85, column 5.
   def objectsOtherThan = getAnnotationObjects() - myObjectsWithinAList
       ^

3 errors
carlocastoldi commented 7 months ago

is there a particular reason to why loadWarpedAtlasAnnotations() does not return the Root annotation?

Not really, I think it could return the root annotation without any problem.

I did it, in the end

should we fix the tests so that they're actually useful?

Sure, if you know how to do that, that'd be nice.

Perhaps this can be done in a future PR. I feel like it's something more important for the long-term support of this extension, rather than for immediate need

do you think we should make LoadAtlasRoisToQuPathCommand be a little be more complex? But more importantly, what do you have in mind in terms of complexity?

I'd say the ability to display and choose between available atlas alignments

NicoKiaru commented 6 months ago

Oups, missed your replies, you were too fast...

I feel like this shouldn't be the work of a script shipped with the extension. For that the website/documentation is perhaps the better place?

Yes that's right. We can remove the script. We could add a command that would open the documentation instead. And the documentation needs to be rewritten.

I'd say the ability to display and choose between available atlas alignments

Wait, you have several alignments for the same image ?

carlocastoldi commented 6 months ago

Wait, you have several alignments for the same image ?

Not personally, no. But I've helped other laboratories that work on non-adult mice. Since their mice don't have an atlas perfectly designed on their age, they have to choose the lesser evil between the younger and the older one.

For this, they wanted to import both and compare the atlases on the single images, side by side

carlocastoldi commented 6 months ago

I beg your pardon for askin, but is there any reason this wasn't merged yet? :o

NicoKiaru commented 6 months ago

No reason, I asked above for your formal 'OK' and if you wanted to add additional stuff but this got lost with the other posts. But I suppose it's all ok on your side and there's nothing to add, so let's go for it.

carlocastoldi commented 6 months ago

Thank you!