Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support connection to LIMS #93

Closed
jcfr opened this issue Jun 4, 2019 · 48 comments
Closed

Support connection to LIMS #93

jcfr opened this issue Jun 4, 2019 · 48 comments
Assignees
Labels
Size: M Status: Awaiting Response Waiting for a response/more information Type: Feature Request Request for a new feature
Milestone

Comments

@jcfr
Copy link
Collaborator

jcfr commented Jun 4, 2019

What's the problem this feature will solve?

It should be possible to start CellLocator from LIMS.

Describe the solution you'd like

CellLocator will be associated with a file extension (e.g .cell-locator.allen-lims). When double clicking on the file, cell locator would be started.

Additional context
See also #21

@jcfr jcfr added Status: Awaiting Response Waiting for a response/more information Type: Feature Request Request for a new feature labels Jun 4, 2019
@jcfr jcfr added this to the 0.2.0 milestone Jun 4, 2019
@wbwakeman
Copy link

wbwakeman commented Mar 19, 2020

The team implementing a LIMS API for CellLocator to work with baked in a validation mechanism that makes it important to track the expected number of pinned locations for a particular (slice) specimen. The number of pins in a JSON posted to the API must match that number. If a user intends to add a pin, then the Cell Locator will need to use another API call to first increment the number of expected cells.

The 'kind' of data that tracks the number of expected cells is labeled: "IVSCC expected cell count".
The 'kind' of data that tracks the pinned location of cells is labeled: "IVSCC cell locations".

The base URL of the calls should be configurable. Our production system URL is: http://lims2 . My dev system URL is http://ibs-waynew-vm1:3000 (for example).

Will add additional comments with API spec

@wbwakeman
Copy link

Fetch Specimen Metadata

Returns json metadata of a specimen based on the kind of metadata.
URL

/specimen_metadata/view
Method

GET
URL Params
Required:

specimen_id=[integer] - Unique id property of the Specimen entry in the LIMS that the metadata is associated with
kind=[string] - The kind of metadata that is to be retrieved (eg. "IVSCC cell locations")

Optional:

version_number=[integer] - The version of the metadata, which may be defined by the kind of metadata

Success Response

Code: 200 - Content: { "version_number": 2, "data": { "foo" : "bar" } }

Error Responses

Code: 500 - Content: { "status": 500, "message": "Something went wrong" }
Code: 404 - Content: { "status": 404, "message": "Record not found" }
Code: 400 - Content: { "status": 400, "message": "Param foo is missing" }
message values in the error response are examples

Sample Call

/specimen_metadata/view?specimen_id=1234&kind=IVSCC%20cell%20locations

/specimen_metadata/view?specimen_id=1234&kind=IVSCC%20cell%20locations&version_number=3
Notes

If version_number is not specified, then the current version will be returned

@wbwakeman
Copy link

wbwakeman commented Mar 19, 2020

Store Specimen Metadata

Stores json metadata of a specimen based on the kind of metadata.
URL

/specimen_metadata/store
Method

POST
URL Params

NONE
If URL params are provided, the request will be rejected with code 400

Data Params - POST body
Required:

specimen_id=[integer] - Unique id property of the Specimen entry in the LIMS that the metadata is associated with
kind=[string] - The kind of metadata that is to be retrieved (eg. "IVSCC cell locations")
data=[object] - The metadata to be stored - this should be a valid JSON object

Example Body:

{
    "specimen_id": 1234,
    "kind": "IVSCC cell locations",
    "data": {
        "foo": "bar"
    }
}

Example Body:

{
    "specimen_id": 1234,
    "kind": "IVSCC expected cell count",
    "data": 2
}

Required Headers

Content-Type: application/json

Success Response

Code: 200 - Content: { "id": 1234, "version_number": 2 }

Error Responses

Code: 500 - Content: { "status": 500, "message": "Something went wrong" }
Code: 404 - Content: { "status": 404, "message": "Record not found" }
Code: 400 - Content: { "status": 400, "message": "Param foo is missing" }
message values in the error response are examples

Sample Call

/specimen_metadata/store

@jcfr
Copy link
Collaborator Author

jcfr commented Mar 26, 2020

  • Where does the specimen_id come from ?  Should it be a parameter passed when starting the application ?

  • What are the possible value for "kind" ? In comment Support connection to LIMS #93 (comment), the same sentence is duplicated. Should I used IVSCC cell locations or IVSCC expected cell count

Note: You can format content easily, you can for example see how I ensured the json content is properly highlighted trying to edit this comment: #93 (comment)

@wbwakeman
Copy link

Hi @jcfr I corrected the duplicated sentence in the comment above.

The specimen_id will come in a JSON that is specified when opening Cell Locator. I am trying to capture two related cases here. In one of them, we will pass in a stub with just the specimen_id. Can you propose a location in the JSON file to put the id? (and an associated name)?

Summaries of the two cases:
PatchSeq Mouse electrophysiology case:

  • Pinning occurs during experiment acquisition
  • slice specimen exists in LIMS
  • No cell specimen exists in LIMS yet
  • WSE knows slice specimen name and id
  • WSE checks LIMS for existing pins
  • For each cell they record from, WSE will:
    • check slice specimen for existing metadata
    • check slice specimen for cell count
    • increment cell count
    • query LIMS for angle and plane
    • Launch Cell Locator with a stub containing slice specimen id and arguments for plane and angle
      • Cell Locator opens to appropriate location for the plane and angle
      • Cell Locator displays the specimen id (and name?)
    • User draws polygon/places pin in Cell Locator
  • User updates again and saves OR closes program and indicates to WSE to move to next step

PatchSeq Mouse morphology case:

  • Pinning occurs long after experiment acquisition
  • slice specimen exists in LIMS
  • cell specimens exists in LIMS
  • User finds the slice they want in LIMS, clicks on "Launch Cell Locator" link for that slice specimen
    • LIMS downloads a file with an extension that has been associated with the Cell Locator tool
    • Double-clicking on that file launches cell locator with all of the cells for the slice pinned, and at the specified view for the first cell
    • Users can modify existing pinned location
    • Clicking save will POST to LIMS with the updated pinned locations
    • Clicking 'New pin' will POST to LIMS to increment the cell count
    • User closes Cell Locator.
    • Maybe consider if there is unsaved work, prompt the user to save.

@jcfr
Copy link
Collaborator Author

jcfr commented Apr 9, 2020

Thanks for providing these additional details. It will allow to move forward.

I will get back to you regarding where in the json the specimen_id should be.

In the meantime, could you provide more details regarding the POST request that will be submitted to LIMS, what are the exact data expected to be submitted ?

@wbwakeman
Copy link

This example has the cell locator information for specimen_id 1234 in the 'data' part of the JSON:
https://gist.github.com/wbwakeman/b22ecf8297d3424b4d1b005e790b7c55

This could be posted to http://lims2/specimen_metadata/store to update the cell locations for that specimen.

@jcfr jcfr added the Size: M label Jun 11, 2020
@jcfr
Copy link
Collaborator Author

jcfr commented Jun 30, 2020

@wbwakeman
Copy link

For now, URL could be added to https://github.com/BICCN/cell-locator/blob/master/Applications/CellLocatorApp/Resources/Settings/DefaultSettings.ini

Is this a file that a Windows user has access to after they install the program?

I tried to find that file on my installation but could not.

@jcfr
Copy link
Collaborator Author

jcfr commented Jun 30, 2020

This file is used to initialize the application settings saved in your home folder.
See https://slicer.readthedocs.io/en/latest/user_guide/settings.html#information-for-advanced-users

In the case of CellLocator the settings file would be in C:\Users\<your_user_name>\AppData\Roaming\Allen Institute

@allemangD
Copy link
Collaborator

For development purposes, I've created a small flask application to mock the behavior of the LIMS api locally. I'd like to verify that my behavior is correct: If I make a valid POST request to /specimen_metadata/store with body

{
  "kind": "IVSCC cell locations",
  "specimen_id": 1234,
  "data": {"foo": "bar"}
}

Then I should expect a GET request to /specimen_metadata/view?kind=IVSCC+cell+locations&specimen_id=1234 to have response

{
  "version_number": 2,
  "data": {
    "kind": "IVSCC cell locations",
    "specimen_id": 1234,
    "data": {"foo": "bar"}
  }
}

Specifically, the path to the markup data in the result is nested by two "data" keys. Is this correct?


Regarding LVSCC expected cell count: is the value for this expected to differ from Markups_Count in the main body? I don't have it done yet, but it should be straightforward to make the request with that value any time the data is stored.


I've been making HTTP requests with urllib2. I had done some tentative implementations using qt.QNetworkAccessManager, but the urllib2 code is much more straightforward.

Advantages to the qt implementation would be asynchronous network calls, and we won't have to change that code when Python 3 comes around. @jcfr, do you have any thoughts on this?

You can see my changes in a branch on my fork here: https://github.com/allemangd-forks/cell-locator/tree/lims

Currently, all the behavior is enabled when a --lims-id flag is given to the program. A better name for the flag might be --lims-specimen-id, or it may be preferable to use a different mechanism to load the data. The changes to add this flag are in my fork of Slicer, here: https://github.com/allemangd-forks/Slicer/tree/lims

@jcfr
Copy link
Collaborator Author

jcfr commented Jul 2, 2020

I've been making HTTP requests with urllib2. I had done some tentative implementations using qt.QNetworkAccessManager, but the urllib2 code is much more straightforward.

I suggest you look into https://github.com/commontk/qRestAPI, it is already available in Slicer.

There is a test illustrating how it could be used: https://github.com/commontk/qRestAPI/blob/master/Testing/qMidasAPITest.cpp

It is not expose in python, but would be easy to do so.

allemangD added a commit to KitwareMedical/AllenInstituteMockLIMS that referenced this issue Jul 3, 2020
@allemangD
Copy link
Collaborator

allemangD commented Jul 3, 2020

Just for now, I've just uploaded my mock app to my own repo; allemangD/lims-mock. I'll transfer this to a more appropriate user or organization, but I'm unsure where exactly it should belong. @wbwakeman, could you verify that this correctly mimics the behavior of LIMS like we discussed in our meeting yesterday?

@allemangD
Copy link
Collaborator

The mock lims app has been transfered to the KitwareMedical organization:

KitwareMedical/AllenInstituteMockLIMS

@wbwakeman
Copy link

@jcfr @allemangD I had a chance today to compare the behavior of your mock LIMS API with my test system. Here is what difference that I notice
image
The 'data' payload of the GET call for "IVSCC cell locations" returns just the location data on the LIMS system. Your spec shows that it returns 'specimen_id', 'kind' and another 'data' with the payload in it.

@wbwakeman
Copy link

Here is a related item that just probably was not clear. Running the GET with the 'IVSCC expected cell count" kind returns just an integer in the 'data' payload:

http://ibs-waynew-vm1:3000/specimen_metadata/view?specimen_id=1234&kind=IVSCC%20expected%20cell%20count

{
"version_number": 0,
"data": 1
}

@wbwakeman
Copy link

One other note too just FYI, the POST that adds new cell locations will do validation to ensure that there is a Markups field under 'data' that contains an array of cell locations

A valid input is available here:
https://gist.github.com/wbwakeman/b22ecf8297d3424b4d1b005e790b7c55

allemangD added a commit to KitwareMedical/AllenInstituteMockLIMS that referenced this issue Jul 22, 2020
jcfr pushed a commit that referenced this issue Jul 30, 2020
This commit updates CellLocator and Slicer adding support for flags `--lims-specimen-id`
and `--lims-base-url`.

If `--lims-specimen-id` is provided, it is used:
* At application startup time to
   (1) retrieve the corresponding annotation from LIMS and load it by using the `/specimen_metadata/view` endpoint.
   (2) enable the "Upload Annotation" button
* After user initiate annotation upload to LIMS while using the `/specimen_metadata/store` endpoint.

The "kind" parameter for both  endpoints `/specimen_metadata/view` and `/specimen_metadata/store`
is set to "IVSCC cell locations"

IVSCC expected cell count is currently unsupported.

For testing this functionality, a mock server as been implemented. Relevant instructions
are available here: https://github.com/KitwareMedical/AllenInstituteMockLIMS

List of Slicer changes:

$ git shortlog 0661948e5..9b00dc521f --no-merges
David Allemang (1):
      [cell-locator] ENH: Add command line args --lims-specimen-id and --lims-base-url for LIMS integration.
jcfr pushed a commit that referenced this issue Jul 30, 2020
This was clarified in discussion for #93.
@jcfr jcfr added the qa: biccn Issue (resolved) -> Kitware (QA done), biccn (QA in progress) label Aug 7, 2020
@jcfr
Copy link
Collaborator Author

jcfr commented Aug 19, 2020

Interestingly, we now have a different message.

Instead of:

Failed to connect to LIMS server

we have

Failed to load specimen ID 1029224009 from LIMS server.

Consider changing:

logging.error('Failed to load specimen ID %s from LIMS server.', specimenID)

into

logging.error('Failed to load specimen ID %s from LIMS server. [status: %s,  URL:%s]' %  (specimenID, res.getcode(), url_with_params))

@wbwakeman
Copy link

Now we're getting somewhere:

Loading LIMS specimen id 1029224009
Failed to load specimen ID 1029224009 from LIMS server. [status: 404,  URL:http://ibs-waynew-vm1:3000/specimen_metadata/view?specimen_id=1029224009&kind=IVSCC+cell+locations]

This is expected, because there is not yet any specimen_metadata records for this specimen (that is what we are going to save as a result of this process).
From the spec above (although actual message text is different):

Code: 404 - Content: { "status": 404, "message": "Couldn't find SpecimenMetadata" }

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 19, 2020

Moving forward, we will make sure to log the status code and message.

In the meantime, is there anything we need to change regarding the endpoints we call ?

if yes, could you suggest change to https://github.com/KitwareMedical/AllenInstituteMockLIMS#endpoints

@wbwakeman
Copy link

I will have a look at the endpoints page.

I guess the better log message would be "Failed to load any annotations for specimen ID " instead of "Failed to load specimen ID".

The next issue is trying to save something. Now that I have opened Cell Locator and it knows what specimen I am working with (even though there are not yet any annotations stored in the database), I want to see the specimen id listed in the Cell Locator and I want to click on 'Save to LIMS'. This is the log when I try to save now:

Upload Annotation Button Clicked
Saving LIMS specimen id 1029224009
Failed to connect to LIMS server

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 19, 2020

To get more details, locally apply similar changes to function saveLIMSSpecimen:

def saveLIMSSpecimen(self, annotationNode, specimenID):
logging.info('Saving LIMS specimen id %s', specimenID)
data = self.saveAnnotationJson(annotationNode)
base = slicer.app.commandOptions().limsBaseURL or 'http://localhost:5000/'
path = '/specimen_metadata/store'
url = urlparse.urljoin(base, path)
body = json.dumps({
'kind': 'IVSCC cell locations',
'specimen_id': specimenID,
'data': data
})
try:
res = urllib2.urlopen(url, data=body)
except urllib2.URLError as e:
logging.error('Failed to connect to LIMS server')
return
if res.getcode() != 200:
logging.error('Failed to store specimen ID %s to LIMS server.', specimenID)

I want to see the specimen id listed in the Cell Locator

Few options:

  1. Display ID in title bar:

image

  1. Display in text a text box under the Save to LIMS button:

image

  1. Display as a tooltip associated with "Save to LIMS" button:

image

I guess the better log message would be "Failed to load any annotations for specimen ID"

Sounds good, we will use this message instead.

@wbwakeman
Copy link

I attempted that, but I've got some syntax incorrect, because there is a slight different in the arguments for those two.

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:/Program Files/CellLocator 0.1.0-2020-07-30/bin/../lib/CellLocator-4.11/qt-scripted-modules/Home.py", line 350
    url_with_params = '%s?%s' % (url, data=body)
                                          ^
SyntaxError: invalid syntax

My attempt:

    try:
      #res = urllib2.urlopen(url, data=body)                                  #WBW b
      url_with_params = '%s?%s' % (url, data=body) 
      res = urllib2.urlopen(url_with_params)
    except urllib2.URLError as e:
      #logging.error('Failed to connect to LIMS server')
      logging.error('Failed to connect to LIMS server: %s' % url_with_params) #WBW
      return

@wbwakeman
Copy link

For "specimen id listed in the Cell Locator", I vote for option #2, the text box.

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 19, 2020

To get more details, locally apply similar changes to function

The following should be implemented then:

    try:
      res = urllib2.urlopen(url, data=body)
    except urllib2.URLError as e:
-      logging.error('Failed to connect to LIMS server')
+     logging.error('Failed to connect to LIMS server. [url: %s, code: %s, reason: %s]' % (url, e.code, e.reason))
+     logging.error("payload: %s" % body)
      return

    if res.getcode() != 200:
-      logging.error('Failed to store specimen ID %s to LIMS server.', specimenID)
+      logging.error('Failed to store specimen ID %s to LIMS server. [code: %s, url: %s]' % (specimenID, res.getcode(), url))

For "specimen id listed in the Cell Locator", I vote for option #2, the text box.

Sounds good.

Next question: When the application is started with LIMS integration, I suggest we hide button like "New/Save/SaveAs/Load" and only keep "Save to LIMS". I anticipate this would allow to keep the UI simple and avoid confusion.

What do you think ?

@wbwakeman
Copy link

Hmmm...I don't get it. This is the new output:

Switch to module:  "Home"
Loading LIMS specimen id 1029224009
Failed to load specimen ID 1029224009 from LIMS server. [status: 404,  URL:http://ibs-waynew-vm1:3000/specimen_metadata/view?specimen_id=1029224009&kind=IVSCC+cell+locations]
Upload Annotation Button Clicked
Saving LIMS specimen id 1029224009
Failed to connect to LIMS server. [url: http://ibs-waynew-vm1:3000/specimen_metadata/store, code: 400, reason: Bad Request]
payload:{"specimen_id": 1029224009, "kind": "IVSCC cell locations", "data": {"Markups_Count": 1, "TextList_Count": 0, "Locked": 0, "DefaultRepresentationType": "spline", "DefaultOntology": "Structure", "DefaultReferenceView": "Coronal", "DefaultCameraPosition": [5875.669410294076, -50329.33493515295, -4026.159568033225], "DefaultThickness": 50.0, "Markups": [{"CameraViewUp": [0.0, 0.0, 1.0], "Locked": 0, "Description": "", "StepSize": 24.999999999999996, "OrientationWXYZ": [0.0, 0.0, 0.0, 1.0], "CameraPosition": [5875.669410294076, -50329.33493515295, -4026.159568033225], "Selected": 1, "Thickness": 50.0, "ReferenceView": "Coronal", "Label": "Annotation-1", "RepresentationType": "spline", "Points": [{"y": -7462.880264802354, "x": 8763.0690376569, "z": -1083.374510970828}, {"y": -7267.639578301313, "x": 9764.742677824266, "z": -1721.9780216473955}, {"y": -7170.019235050791, "x": 8953.864016736394, "z": -2041.2797769856843}, {"y": -7114.236181764779, "x": 7952.190376569033, "z": -2223.7379228932796}], "SplineOrientation": [1.0, 0.0, 0.0, 5686.499999999999, 0.0, -0.29237170472273677, 0.9563047559630355, -6574.999999999999, 0.0, 0.9563047559630355, 0.29237170472273677, -3987.4999999999995, 0.0, 0.0, 0.0, 1.0], "Closed": 1, "Points_Count": "4", "AssociatedNodeID": "vtkMRMLModelNode5", "Ontology": "Structure", "ID": "vtkMRMLMarkupsSplinesNode_0", "Visibility": 1}], "DefaultSplineOrientation": [1.0, 0.0, 0.0, 5686.499999999999, 0.0, -0.29237170472273677, 0.9563047559630355, -6574.999999999999, 0.0, 0.9563047559630355, 0.29237170472273677, -3987.4999999999995, 0.0, 0.0, 0.0, 1.0], "DefaultCameraViewUp": [0.0, 0.0, 1.0], "MarkupLabelFormat": "%N-%d", "TextList": [null], "DefaultStepSize": 24.999999999999996}}

When I drop the same JSON into a REST tool ("Insomnia") and make the same POST to the same system, it succeeds.

Regarding hiding "New/Save/SaveAs/Load" when the tool was launched using --lims-base-url, I think that is a good idea.

@wbwakeman
Copy link

Does the POST include the required header: Content-Type: application/json ?

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 20, 2020

Good point. Thanks for testing 🙏

We will improving the error handling and switch to using requests to properly deal with header setting.

We will also update the mock application to expect a json pay load to ensure local testing work as expected.

Once this is done, we will get back to you for an other round of QA.

@allemangD
Copy link
Collaborator

@jcfr I've addressed the feedback above and created #133 with those changes.

@wbwakeman
Copy link

@jcfr @allemangD I tried the new version and it looks like we are progressing. Here is the output from the console:

Loading LIMS specimen id 1029224009
Failed to load annotations for LIMS specimen 1029224009. Error 404: "Couldn't find SpecimenMetadata"
Upload Annotation Button Clicked
Saving LIMS specimen id 1029224009
Failed to save annotations for LIMS specimen 1029224009. Error 400: 'Data must include a "Markups" key containing an array of cell locations'

It does seem like the format of the saved output has changed. Based on the previous format, our developer put in a validation for the "Markups" key, as described in the July 13th comment and with an example in this gist. Let me know if you can change your output format to match, or if we need to update our output validation code.

@allemangD
Copy link
Collaborator

I see that the current implementation places the annotations in the key "markups", lower-case. This explains the missing key error, and is an easy fix.

What validation is done for each element of that "Markups" list? Those objects are where the most significant changes to the format occurred, due to us using the new markups module in Slicer, and would be more difficult to change back. All the keys at the top-level of the file are easy to change.

To change the key used locally, you can change lines 239 and 256 in the same file, lib/CellLocator-4.11/qt-scripted-modules/Home.py.

def toDict(self):
"""Convert this collection to dict representation, suitable for json serialization."""
return {
'markups': [annotation.toDict() for annotation in self.annotations],
'currentId': self.currentId,
'referenceView': self.referenceView,
'ontology': self.ontology,
'stepSize': self.stepSize,
'cameraPosition': self.cameraPosition,
'cameraViewUp': self.cameraViewUp,
}
@classmethod
def fromDict(cls, data):
"""Convert a dict representation to annotation collection, suitable for json deserialization."""
manager = cls()
manager.annotations = [Annotation.fromDict(item) for item in data['markups']]
manager.currentId = data['currentId']
manager.referenceView = data['referenceView']
manager.ontology = data['ontology']
manager.stepSize = data['stepSize']
manager.cameraPosition = data['cameraPosition']
manager.cameraViewUp = data['cameraViewUp']
return manager

diff --git a/Modules/Scripted/Home/Home.py b/Modules/Scripted/Home/Home.py
index 368839a..77dea97 100644
--- a/Modules/Scripted/Home/Home.py
+++ b/Modules/Scripted/Home/Home.py
@@ -236,7 +236,7 @@ class AnnotationManager:
     """Convert this collection to dict representation, suitable for json serialization."""
 
     return {
-      'markups': [annotation.toDict() for annotation in self.annotations],
+      'Markups': [annotation.toDict() for annotation in self.annotations],
       'currentId': self.currentId,
 
       'referenceView': self.referenceView,
@@ -253,7 +253,7 @@ class AnnotationManager:
 
     manager = cls()
 
-    manager.annotations = [Annotation.fromDict(item) for item in data['markups']]
+    manager.annotations = [Annotation.fromDict(item) for item in data['Markups']]
     manager.currentId = data['currentId']
 
     manager.referenceView = data['referenceView']

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 28, 2020

Thanks for the feedback.

There is now a PR under review documenting the format: #145

Let me know if you can change your output format to match, or if we need to update our output validation code.

To keep the code base simple, I think updating the validation code is the most sensible things to do.

Keeping all key lower case makes sense.

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 28, 2020

To change the key used locally,

For sake of testing locally, applying the suggestion of @allemangD make sense 👍

@wbwakeman
Copy link

The only other validation that we are doing is a count of the number of elements underneath the "Markups" key. We will check that count to see that it matches what is in the LIMS database.
For this example that does appear to be the case, so that is okay.
I will look into changing the case of our validation so that "markups" is acceptable.

@wbwakeman
Copy link

After making that change, it saves to LIMS :-)
I'll poke around some more with it, but that is very promising

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 28, 2020

I am also thinking we should remove some properties from the annotation file and have there initialized at loading by the application.
The idea is to save to LIMS only the relevant information.

@wbwakeman What do you think ? See below for the proposed simplification
@allemangD Do you anticipate any issues doing so ?

Other information we should probably include in the JSON under a metadata dictionnary:

  • version of the CCF
  • version of cell-locator

Also which terminologies to you use in LIMS ? I think we should change the markups and markup key to properly reflect the used terminology.

diff --git a/annotation.json b/annotation-cleaned.json
index 786ee63..e6ae130 100644
--- a/annotation.json
+++ b/annotation-cleaned.json
@@ -2,16 +2,9 @@
     "markups": [
         {
             "markup": {
-                "type": "ClosedCurve",
                 "coordinateSystem": "LPS",
-                "locked": false,
-                "labelFormat": "%N-%d",
                 "controlPoints": [
                     {
-                        "id": "1",
-                        "label": "MarkupsClosedCurve-1",
-                        "description": "",
-                        "associatedNodeID": "vtkMRMLScalarVolumeNode1",
                         "position": [
                             -7725.476777936878,
                             5071.924649397559,
@@ -28,16 +21,8 @@
                             0,
                             1
                         ],
-                        "selected": true,
-                        "locked": false,
-                        "visibility": true,
-                        "positionStatus": "defined"
                     },
                     {
-                        "id": "2",
-                        "label": "MarkupsClosedCurve-2",
-                        "description": "",
-                        "associatedNodeID": "vtkMRMLScalarVolumeNode1",
                         "position": [
                             -8785.61316343005,
                             5514.035987881592,
@@ -54,16 +39,8 @@
                             0,
                             1
                         ],
-                        "selected": true,
-                        "locked": false,
-                        "visibility": true,
-                        "positionStatus": "defined"
                     },
                     {
-                        "id": "3",
-                        "label": "MarkupsClosedCurve-3",
-                        "description": "",
-                        "associatedNodeID": "vtkMRMLScalarVolumeNode1",
                         "position": [
                             -7732.355384361564,
                             5984.77667260136,
@@ -80,16 +57,8 @@
                             0,
                             1
                         ],
-                        "selected": true,
-                        "locked": false,
-                        "visibility": true,
-                        "positionStatus": "defined"
                     },
                     {
-                        "id": "4",
-                        "label": "MarkupsClosedCurve-4",
-                        "description": "",
-                        "associatedNodeID": "vtkMRMLScalarVolumeNode1",
                         "position": [
                             -6980.525470132225,
                             5682.868336976309,
@@ -106,16 +75,8 @@
                             0,
                             1
                         ],
-                        "selected": true,
-                        "locked": false,
-                        "visibility": true,
-                        "positionStatus": "defined"
                     },
                     {
-                        "id": "5",
-                        "label": "MarkupsClosedCurve-5",
-                        "description": "",
-                        "associatedNodeID": "vtkMRMLScalarVolumeNode1",
                         "position": [
                             -6735.759199784232,
                             5461.593738502808,
@@ -132,49 +93,8 @@
                             0,
                             1
                         ],
-                        "selected": true,
                         ],
-                        "selected": true,
-                        "locked": false,
-                        "visibility": true,
-                        "positionStatus": "defined"
                     }
                 ],
-                "display": {
-                    "visibility": true,
-                    "opacity": 1,
-                    "color": [
-                        0.4,
-                        1,
-                        1
-                    ],
-                    "selectedColor": [
-                        1,
-                        0.5000076295109483,
-                        0.5000076295109483
-                    ],
-                    "propertiesLabelVisibility": true,
-                    "pointLabelsVisibility": false,
-                    "textScale": 3,
-                    "glyphType": "Sphere3D",
-                    "glyphScale": 1,
-                    "glyphSize": 5,
-                    "useGlyphScale": true,
-                    "sliceProjection": false,
-                    "sliceProjectionUseFiducialColor": true,
-                    "sliceProjectionOutlinedBehindSlicePlane": false,
-                    "sliceProjectionColor": [
-                        1,
-                        1,
-                        1
-                    ],
-                    "sliceProjectionOpacity": 0.6,
-                    "lineThickness": 0.2,
-                    "lineColorFadingStart": 1,
-                    "lineColorFadingEnd": 10,
-                    "lineColorFadingSaturation": 1,
-                    "lineColorFadingHueOffset": 0,
-                    "handlesInteractive": false,
-                    "snapMode": "toVisibleSurface"
-                }
             },
             "orientation": [
                 0.9423723483536421,

@wbwakeman
Copy link

Regarding simplifying the JSON that is saved, so long as it still captures all the important information then I support that. Not sure if I am well-qualified to say what all is important.

I am okay with having some number of 'markup' entries under the 'markups' key. Say, how does this play into the request for multiple annotations per file #90 ?

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 28, 2020

so long as it still captures all the important information then I support that

Great. All the keys I removed are related to colors and internal details specific to the application.

how does this play into the request for multiple annotations per file #90 ?

markups is a list of markup:

    "markups": [
        {
            "markup": {
                "coordinateSystem": "LPS",
                "controlPoints": [
                    [...]
                ],
            },
            "orientation": [
                0.9423723483536421,
                -0.10260749019479301,
                -0.3184431817677528,
                [...]
                1
            ],
            "representationType": "spline",
            "thickness": 50
        }
    ],

We will make sure to have each markup identified with a key.

Questions:

  • From the LIMS perspective should we use specific identifier for each "markup" ? Or numbering them in the order of creation is sufficient ?

  • Should we keep the terminology "markups / markup" or change it to "annotations / annotation" ?

@wbwakeman
Copy link

From the LIMS perspective should we use specific identifier for each "markup" ? Or numbering them in the order of creation is sufficient ?

1-based indexing would work for me. Is there any way for users to add their own text that goes along with an annotation?

Should we keep the terminology "markups / markup" or change it to "annotations / annotation" ?

I'm not too fussed either way. "annotations / annotation" probably has broader appeal to scientific users. Just let me know if you change it; I will have to make a matching change on my side.

w.r.t to multiple annotations, I guess this is enabled now? #90 is just adding the GUI components so that users can manage more than one annotation? (e.g. "Now I am done modifying this annotation, and want to start adjusting the other one...")

@jcfr
Copy link
Collaborator Author

jcfr commented Aug 28, 2020

1-based indexing would work for me. Is there any way for users to add their own text that goes along with an annotation?

@allemangD and I were wondering if that would be useful. For now, annotation are called "Markup", "Markup-1", ... and user will have the possibility to edit the text directly within the list.

"annotations / annotation" probably has broader appeal to scientific users.

We will be using "annotation" and "annotations", looking at the presentation to the stakeholder as well as the poster presented to SFN, the word annotate and annotation is a recurrent theme.

@danielsf Any suggestions ?

multiple annotations, I guess this is enabled now?

Yes, the infrastructure now nicely support that. @allemangD is currently finalizing the integration and it will be available in the next release. Likely middle of next week, but I will let David comment regarding the timeline.

@allemangD
Copy link
Collaborator

I will let David comment regarding the timeline.

Yes, I think it should be ready for QA in the next couple days, and could be finalized by mid-next week.

@jcfr jcfr closed this as completed Sep 13, 2020
@jcfr jcfr removed the qa: biccn Issue (resolved) -> Kitware (QA done), biccn (QA in progress) label May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: M Status: Awaiting Response Waiting for a response/more information Type: Feature Request Request for a new feature
Projects
None yet
Development

No branches or pull requests

3 participants