search

spinalcordtoolbox / manual-correction

Scripts for the manual correction of spinal cord labels
MIT License
4 stars 0 forks source link

Add `"Name"` and `"SpatialReference"` key-value pair to JSON sidecar, remove `check_if_modified ` #76

Closed valosekj closed 9 months ago

valosekj commented 10 months ago

Updated description based on https://github.com/spinalcordtoolbox/manual-correction/pull/76#issuecomment-1939082475:

This PR adds new key-value pairs SpatialReference': 'orig' and 'Name': 'Manual' into JSON sidecars:

{
  "SpatialReference": "orig",
  "GeneratedBy": [
    {
      "Name": "Manual",
      "Author": "Test Rater",
      "Date": "2024-02-14"
    }
  ]
}

The PR also removes the check_if_modified function; for context see https://github.com/spinalcordtoolbox/manual-correction/pull/76#issuecomment-1939147742.

Original description This PR proposes a new key-value pair `"Note"` to track if a label was only visually inspected or manually modified. To determine between these two scenarios, the already existing [`check_if_modified` function](https://github.com/spinalcordtoolbox/manual-correction/blob/7f20117654e6f1a602f144241171c46362ba89dc/manual_correction.py#L486) is levared. ## Scenario 1 If the label was only visually inspected but not modified, add `"Note": "Visually verified"` to the JSON file. Example: ```json { "GeneratedBy": [ { "Author": "Jan Valosek", "Note": "Visually verified", "Date": "2024-01-18 10:42:24" } ] } ``` ## Scenario 2 If the label was corrected, add `"Note": "Manually corrected"`: ```json { "GeneratedBy": [ { "Author": "Jan Valosek", "Note": "Manually corrected", "Date": "2024-01-18 10:42:24" } ] } ``` Relevant discussion: https://github.com/ivadomed/canproco/issues/73#issuecomment-1911206492
NathanMolinier commented 10 months ago

Hey @valosekj ! Thanks for opening this PR ! I was actually thinking about updating some fields of our json sidecars.

Regarding your proposition, I think could just stick to our standards as described here. In other terms, I think we should start using the key "Name" instead of "Note" which is proposed by the BIDS convention as well.

Then regarding the associated values:

See one example for the dataset lumbar-vanderbilt

{
    "SpatialReference": "orig",
    "GeneratedBy": [
        {
            "Name": "Manual",
            "Description": "Manually created by our collaborators"
        },
        {
            "Name": "Quality Control",
            "Author": "Nathan Molinier",
            "Date": "2024-01-12 13:37:17"
        }
    ]
}

Also, as shown in the previous example, we should also start adding the key "SpatialReference" .

Note: we should also make sure that other fields like "SpatialReference" are not removed when using manual_correction.py

valosekj commented 10 months ago

Points based on an in-person discussion with @NathanMolinier:

Remaining points to discuss:

Values to use in the JSON sidecars for the key Name.

Proposed by @NathanMolinier:

Proposed by Jan:

NathanMolinier commented 9 months ago

Regarding "Name": "Manually corrected" I still think that using "Name": "Manual" is a bit better to deal with both corrections and creations. Indeed, when we create labels from scratch having "Manually corrected" is for me a bit strange since the label was not corrected but "created". And having another Name such as "Manually created" will lead to more confusion.

Also, I tested the PR to correct some labels and I think that we should add the field "SpatialReference" when not already present in the JSON sidecar.

valosekj commented 9 months ago

Regarding "Name": "Manually corrected" I still think that using "Name": "Manual" is a bit better to deal with both corrections and creations. Indeed, when we create labels from scratch having "Manually corrected" is for me a bit strange since the label was not corrected but "created". And having another Name such as "Manually created" will lead to more confusion.

Great point! Agree! Changed to "Name": "Manual" in https://github.com/spinalcordtoolbox/manual-correction/pull/76/commits/be73315cf6b30e4a0903c34811bc463b823dd20e

Also, I tested the PR to correct some labels and I think that we should add the field "SpatialReference" when not already present in the JSON sidecar.

Agree. Could you please implement this change and push to this branch?

jcohenadad commented 9 months ago

I would go with the simplest approach: One value: "Manual". Unless there is a VERY good reason for distinguishing QC from manually corrected.

valosekj commented 9 months ago

I would go with the simplest approach: One value: "Manual". Unless there is a VERY good reason for distinguishing QC from manually corrected.

By "Name": "Manual" we meant that the label was manually corrected. While by "Name": "Quality Control" we meant that the label was just visually controlled in HTML QC and considered correct (i.e. label was produced completely automatically, and no manual correction was required). This would help us to distinguish whether the label was generated fully automatically or whether manual intervention was required. Or do you think this type of information is not necessary?

jcohenadad commented 9 months ago

This would help us to distinguish whether the label was generated fully automatically or whether manual intervention was required. Or do you think this type of information is not necessary?

that's exactly my point: how useful is that information?

my perspective is from someone managing 100+ projects over many years. In 5y from now, i imagine the scenario where a student would come in, and ignore the difference between "manual" and "visually qced", and put a wrong label. I prefer less label than wrong labels-- hence my go-for-simple-solution philosophy

NathanMolinier commented 9 months ago

Based on @jcohenadad's comment, I just removed the difference between visually checked and modified.

Also, I added a line to check if the field "SpatialReference" is present or I just add it.

valosekj commented 9 months ago

@NathanMolinier why did you remove the check_if_modified function in https://github.com/spinalcordtoolbox/manual-correction/pull/76/commits/c8fad4ec964b2b6f3458b6371b3c94cadcda4612?

NathanMolinier commented 9 months ago

@NathanMolinier why did you remove the check_if_modified function in https://github.com/spinalcordtoolbox/manual-correction/pull/76/commits/c8fad4ec964b2b6f3458b6371b3c94cadcda4612?

Hey @valosekj ! From what Julien said I removed the code that was making a difference between a modified image and a checked image. Since this function was not used anymore I thought that I could remove it to avoid having dead code since it is still tracked by GitHub.

I could add it back if you want ?

valosekj commented 9 months ago

From what Julien said I removed the code that was making a difference between a modified image and a checked image. Since this function was not used anymore I thought that I could remove it to avoid having dead code since it is still tracked by GitHub.

Yeah, this is a valid point, indeed. But now, without the check_if_modified function, the script will update the JSON sidecar every time the label is opened.

NathanMolinier commented 9 months ago

Yes, but that's what we want I guess. The user intentionally added the filename to the YML config

valosekj commented 9 months ago

Yes, but that's what we want I guess. The user intentionally added the filename to the YML config

Right. Okay, let's keep check_if_modified removed. Can you please update unit tests to pass with your latest changes? I will then merge.

NathanMolinier commented 9 months ago

Yes, but that's what we want I guess. The user intentionally added the filename to the YML config

Right. Okay, let's keep check_if_modified removed. Can you please update unit tests to pass with your latest changes? I will then merge.

Yes sure !

valosekj commented 9 months ago

Great, thanks @NathanMolinier! 👍🏻 Merging!