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

[valosekj]

Updated description based on #76 (comment):

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 #76 (comment).

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 is levared.

Scenario 1

If the label was only visually inspected but not modified, add "Note": "Visually verified" to the JSON file. Example:

{
    "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":

{
    "GeneratedBy": [
        {
            "Author": "Jan Valosek",
            "Note": "Manually corrected",
            "Date": "2024-01-18 10:42:24"
        }
    ]
}

Relevant discussion: ivadomed/canproco#73 (comment)

[NathanMolinier]

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:

• for manual corrected images I believe that using "Manual" is more BIDS compatible.
• for quality controlled images, I already pushed images with the value "Quality Control" in the lumbar-vanderbilt dataset, but for that I am opened to any suggestions.

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]

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

• rename the key Note in this comment to Name - done in ea18915
• if we create labels from scratch, the manual-correction script should add "SpatialReference": "orig", key-value entry to the JSON sidecar - done in bbfb650

Remaining points to discuss:

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

Proposed by @NathanMolinier:

• "Name": "Manual"
• "Name": "Quality Control"

Proposed by Jan:

• "Name": "Manually corrected"
• "Name": "Visually verified"

[NathanMolinier]

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]

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 be73315

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]

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

[valosekj]

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]

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]

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]

@NathanMolinier why did you remove the check_if_modified function in c8fad4e?

[NathanMolinier]

@NathanMolinier why did you remove the check_if_modified function in c8fad4e?

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]

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]

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

[valosekj]

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]

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 !

[NathanMolinier]

Nothing to add on my side

[valosekj]

Great, thanks @NathanMolinier! 👍🏻 Merging!