Kitware / UPennContrast

UPenn ?
https://upenn-contrast.netlify.com/
Apache License 2.0
8 stars 6 forks source link

Add an endpoint for adding multiple annotation property values #655

Closed bruyeret closed 6 months ago

bruyeret commented 6 months ago

Close #550

This PR is based on PR #651 to avoid a conflict It should be merged after

You need to rebuild girder to see the effects of this PR

The workers can use the function addMultipleAnnotationPropertyValues() For convenience, here is the signature and documentation of this function:

    def addMultipleAnnotationPropertyValues(self, entries):
        """
          Save one or multiple computed property values for the specified annotations
          The recursive_dict_of_numbers can be (and usually is) just a number
          :param list entries: A list of property values for an annotation. Each entry is of type { "datasetId": string, "annotationId": string, "values": { [propertyId: string]: recursive_dict_of_numbers } }
        """
bruyeret commented 6 months ago

You just missed a .annotationClient @arjunrajlab, so I changed your commit, is it good for you? I could do something so that the user doesn't have to use the id of the property (that should always be self.propertyId) For example a kind of tree structure like this:

{
  "myDatasetId": {
    "myAnnotationId": 42,
    "anotherAnnotationId": 84,
  },
  "anotherDatasetId": {
    "lastAnnotationId": 2,
  }
}
arjunrajlab commented 6 months ago

Ah okay, that's cool, thanks for catching that! Even in the current structure, I don't think it needs the propertyId, does it? Just the datasetId and the annotationId? The property values are just set with "values", right?

bruyeret commented 6 months ago

As it is done right now, it needs the property id The entries is a list that must look like this:

[
  {
    "datasetId": "my dataset id",
    "annotationId": "my annotation id",
    "values": {
      "my property id": 42
    }
  },
  {
    "datasetId": "maybe the same dataset id",
    "annotationId": "some other annotation id",
    "values": {
      "a 2D property id": { "x": 0, "y": 1 }
    }
  }
]
arjunrajlab commented 6 months ago

Oh I see what you mean… that's the property name, right, not the propertyId code? I still think the current system is fine and matches the other function well. It's perhaps a little wasteful, but not too bad.

bruyeret commented 6 months ago

It's not the property name but the ID, for example 6617bb27c2e0ea61cecb7636

arjunrajlab commented 6 months ago

Oh, that is weird. In the previous call, we didn't need that, right? Here, it's unclear how we would even get the propertyId if the property has not yet been created, no?

bruyeret commented 6 months ago

When the user creates a property in the "Measure Objects" panel, it creates a property and this is its ID that is given to the worker You can see the function to add a single property value right above the function that you wrote:

    def add_annotation_property_values(self, annotation, values):
        """
        Add property values to this annotation
        :param dict annotation: The annotation having the properties
        :param (number|dict) values: The value for this property (a number or a dict of numbers)
        """
        property_values = {self.propertyId: values}
        self.annotationClient.addAnnotationPropertyValues(self.datasetId, annotation['_id'], property_values)

It uses self.propertyId

arjunrajlab commented 6 months ago

Ahhh okay, I see now, sorry. I think your proposed solution makes the most sense then. So sorry it took me a while to understand!

arjunrajlab commented 6 months ago

In that case, it would be like this?

{
  "myDatasetId": {
    "myAnnotationId": 42,
    "anotherAnnotationId": 84,
  },
  "anotherDatasetId": {
    "lastAnnotationId": 2,
  }
}

And you could put in nested dictionaries as well instead of the numbers?

bruyeret commented 6 months ago

Exactly! Should I do this ?

arjunrajlab commented 6 months ago

Yes, that would be great, thank you!

arjunrajlab commented 6 months ago

Bump @bruyeret

bruyeret commented 6 months ago

@arjunrajlab You can merge this PR whenever you want I tested on small samples of data but not at full scale So if you want to test this branch before merging, you can 👍