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

Update cap_anndata_schema.md #118

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

mfutey
Copy link
Contributor

@mfutey mfutey commented May 1, 2024

For MVP-5404

Made the following two updates:

  • added a correct example value for [cellannotation_setname]_metadata
  • changed [cellannotation_setname]--metadata to [cellannotation_setname]_metadata

@@ -1370,12 +1370,12 @@ Key-value pair in the `uns` dictionary
Python dictionary within the `uns` dictionary, with the key the string `[cellannotation_setname]`


#### [cellannotation_setname]--metadata
#### [cellannotation_setname]_metadata
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have the [cellannotation_setname]_metadata field in the schema.
uns consists of constant name cellannotation_metadata which will maps to dict whith [cellannotation_setname] keys.
So the right example is:

uns = {
    "title": blabla,
    "cellannotation_metadata": {
        "cell_type": {"description": blabla,  "annotation_method": blabla...},
        "author_cell_type":{"description": blabla,  "annotation_method": blabla...},
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent point, I updated [cellannotation_setname]_metadata to cellannotation_metadata but used Ugur's suggestion to have the example value be the value only and not include the key

@@ -1370,12 +1370,12 @@ Key-value pair in the `uns` dictionary
Python dictionary within the `uns` dictionary, with the key the string `[cellannotation_setname]`


#### [cellannotation_setname]--metadata
#### [cellannotation_setname]_metadata
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should change to cellannotation_metadata right?

Suggested change
#### [cellannotation_setname]_metadata
#### [cellannotation_metadata]_metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ubyndr it should be [cellannotation_metadata]

Copy link
Contributor

@evanbiederstedt evanbiederstedt May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ubyndr @mfutey @rm1113

Might be confusion above

[cellannotation_setname] is the name of the "labelset" or cell annotation set. e.g. cell_type

cf #118 (comment)

I am guessing the misalignment is that Roman + Evan simplified this earlier. That example is correct.

• We tag all of this uns stuff as cellannotation_metadata.
• Keys within that dictionary are the set names themselves i.e. [cellannotation_setname]

Are we aligned now?

docs/cap_anndata_schema.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ubyndr ubyndr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've suggested two possible changes for your review.

mfutey and others added 2 commits May 1, 2024 17:22
Co-authored-by: Ismail Ugur Bayindir <ubyndr@gmail.com>
@ubyndr ubyndr requested a review from dosumis May 1, 2024 15:30
@evanbiederstedt evanbiederstedt requested a review from rm1113 May 1, 2024 17:58
Copy link
Contributor

@evanbiederstedt evanbiederstedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mfutey mfutey merged commit e0ca4ec into main May 2, 2024
1 check passed
@dosumis
Copy link
Collaborator

dosumis commented May 2, 2024

@evanbiederstedt - Did this just change the schema so that it now differs from the one you are actually using on CAP?

I want something for now that will conform to what CAP currently does so that we can code to it it load data. If you are changing the key name, then (a) please delay until implemented in CAP (b) please, please, please change this to labelsets so that this conforms to the nomenclature in CAS. Absence of alignment is going to cause endless confusion.

(Also worth noting that the problems here are a good illustration of why programatically validateable schemas are good)

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.

5 participants