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

Allow to specify custom metadata to be included in JSON sidecars #80

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

valosekj
Copy link
Member

This PR adds a new -json-metadata flag to allow you to specify a custom JSON file containing metadata to be added to the JSON sidecar of all corrected labels.

This flag is useful, for example, when a label was obtained automatically, and you want to include this information into the JSON sidecar.


Example usage:

python manual_correction.py -path-img ~/data/site-001_2024-02-21/data_processed/ -path-label ~/data/site-001_2024-02-21/data_processed/ -path-out ~/data/site-001/derivatives/labels/ -config sc_seg_to_correct_t2w.yml -json-metadata custom_metadata.json

where custom_metadata.json:

{
    "Name": "sct_deepseg_sc",
    "Version": "SCT v6.2",
    "Date": "2024-02-21"
}

The output JSON sidecar then looks like this:

{
    "SpatialReference": "orig",
    "GeneratedBy": [
        {
            "Name": "sct_deepseg_sc",
            "Version": "SCT v6.2",
            "Date": "2024-02-21"
        },
        {
            "Name": "Manual",
            "Author": "Jan Valosek",
            "Date": "2024-02-21 10:00:28"
        }
    ]
}

Without -json-metadata custom_metadata.json, the JSON sidecar would look like this:

{
    "SpatialReference": "orig",
    "GeneratedBy": [
        {
            "Name": "Manual",
            "Author": "Jan Valosek",
            "Date": "2024-02-21 10:00:28"
        }
    ]
}

I added a lot of people as reviewers just to let you know that this feature has been added :-)

Relevant: #75

… containing metadata to be added to the JSON sidecar of all corrected labels.
…o be compatible with new 'json_metadata' param.
@NathanMolinier
Copy link
Contributor

NathanMolinier commented Feb 21, 2024

Hey @valosekj ! Thanks for adding this new feature, it will help in some situations indeed.

However, even if it could be helpful, let's not forget that this scenario is not ideal. This repository should only be used for manual corrections, not to fix problems generated by other scripts/pipelines.

If i understand correctly, you generated images using sct_run_batch and figured at the end that JSON sidecars should also be present. But instead of adding them after running your pipeline, I believe it would make more sense to generate them while running your pipeline using the library JQ for example. We could also think about adding the JSON sidecar creation to sct_run_batch directly as a new feature.

@valosekj
Copy link
Member Author

valosekj commented Feb 21, 2024

However, even if it could be helpful, let's not forget that this scenario is not ideal. This repository should only be used for manual corrections, not to fix problems generated by other scripts/pipelines.

If i understand correctly, you generated images using sct_run_batch and figured at the end that JSON sidecars should also be present. But instead of adding them after running your pipeline, I believe it would make more sense to generate them while running your pipeline using the library JQ for example. We could also think about adding the JSON sidecar creation to sct_run_batch directly as a new feature.

Completely agree! The proposed PR can be used until spinalcordtoolbox/spinalcordtoolbox#3394 is addressed (which aims to make SCT and sct_run_batch BIDS compatible).
Even then, there will always be some new models that won't be part of the SCT (and thus won't produce proper JSON sidecars).

@naga-karthik
Copy link
Contributor

Thanks for the PR, @valosekj ! The output JSON sidecar is a bit confusing in the sense that, if there are multiple revisions of the labels (i.e. rater1, deepseg, rater2, rater3, etc.), how is the order of the GeneratedBy key determined? Maybe we could add a key order that specifies the chronological order of the updates to the labels?

Ideally, we can show in reverse chronological order, such as:

{
    "SpatialReference": "orig",
    "GeneratedBy": [
        {
            "Name": "Manual",
            "Author": "Naga Karthik",
            "Date": "2024-02-27"
            "Order": 2
        },
        {
            "Name": "Manual",
            "Author": "Jan Valosek",
            "Date": "2024-02-21 10:00:28"
            "Order": 1
        },
        {
            "Name": "sct_deepseg_sc",
            "Version": "SCT v6.2",
            "Date": "2024-02-21"
            "Order": "0"
        }
    ]
}

This tells us that the first there was deepseg_sc, then corrected by Jan, then corrected by Naga (i.e. the latest set of labels are obtained with Naga's corrections)

What do you think?

@NathanMolinier
Copy link
Contributor

Personally, I'm not a fan of this new key order @naga-karthik. I think it is a duplication of the already existing key date (it could also generate unexpected discrepancies with the date). But basically, if I remember correctly, the metadata is always added to the list, so the last update should be the one at the end (bottom) of the GeneratedBy list.

But, to make sure that we are able to retrace the timeline, we should make sure to always add the time to the date field.

@valosekj
Copy link
Member Author

The output JSON sidecar is a bit confusing in the sense that, if there are multiple revisions of the labels (i.e. rater1, deepseg, rater2, rater3, etc.), how is the order of the GeneratedBy key determined?

We use the .append method meaning that with each revision, we append to the end:

json_dict['GeneratedBy'].append({'Name': 'Manual',

I believe that this assures that the order of revisions is chronological.

@naga-karthik
Copy link
Contributor

naga-karthik commented Feb 28, 2024

@NathanMolinier thanks for clarifying! I somehow overlooked the date key. Yes, order is indeed a duplication of date. But even in date, some entries are in yyyy-mm-dd hh:mm:ss format and some are just in yyyy-mm-dd format. Maybe we should standardize this?

@valosekj
Copy link
Member Author

But even in date, some entries are in yyyy-mm-dd hh:mm:ss format and some are just in yyyy-mm-dd format. Maybe we should standardize this?

Good idea! Let's go with more specific yyyy-mm-dd hh:mm:ss.

@valosekj valosekj merged commit e09272a into main Mar 1, 2024
1 check passed
@valosekj valosekj deleted the jv/allow_to_provide_custom_json_metadata branch March 1, 2024 16:11
@valosekj
Copy link
Member Author

valosekj commented Mar 1, 2024

I self-tested and self-reviewed the PR --> merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants