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

Merge changes for issue 781 (Reactivity) #784

Merged
merged 29 commits into from
Nov 7, 2024

Conversation

bcorrie
Copy link
Contributor

@bcorrie bcorrie commented Apr 16, 2024

Closes #781

@javh
Copy link
Contributor

javh commented Aug 12, 2024

@bcorrie will revisit status.

@bcorrie
Copy link
Contributor Author

bcorrie commented Aug 13, 2024

Continued discussion here: #781 (comment) on #781

@bcorrie bcorrie changed the title Merge changes for issue 781 Merge changes for issue 781 (Reactivity) Sep 9, 2024
@javh
Copy link
Contributor

javh commented Sep 9, 2024

From the call:

  • May need further discussion.
  • Tentatively, how do people feel about:
    1. Should we change reactivity_method and reactivity_readout to free text (from an enum)?
    2. Should we leave in Reactivity.reactivity_ref and Rearrangement.reactivity_id?
  • If we resolve the above, can we merge this and drop other related topics?

@bcorrie
Copy link
Contributor Author

bcorrie commented Sep 9, 2024

For the annotation use case, what about having Rearrangement.reactivity_ref as an array (or list) that points to external object IDs (as CURIEs) that denote that a specific chain has some known similarity to an externally measured record. So we aren't reusing Reactivity (with reactivity_id) to capture this, we are just treating it like an annotation for the sequence. So more like v_call which is an array/list of allele calls.

So we might have (based on this example - #781 (comment)):

    "cell_id" : "5ed6859e99011334ac05e847",
    "junction_aa": "CASSLQSSYNSPLHF",
    "v_call": "TRBV11-2",
    "j_call": "TRBJ1-6",
    "reactivity_ref" : "IEDB_RECEPTOR:182992"

Simple... and doesn't convolute our designed use of Reactivity.

If this was a paired chain study, then you would have the alpha chain linked by cell_id:

    "cell_id" : "5ed6859e99011334ac05e847",
    "junction_aa": "CALRTDRGSTLGRLYF",
    "v_call": "TRAV9-2",
    "j_call": "TRAJ18",
    "reactivity_ref" : "IEDB_RECEPTOR:182992"

If you only care about annotated Rearranegement.reactivity_ref that were paired chains (e.g. @bussec), you can determine which rearrangements in the data have such paired chains using this model with a bit of work.

The down side is that you don't know anything about what the receptor in this case is reactive to, so in the ADC you can't ask the ADC to give me all of the chains that have documented reactivity to Insulin. You could find it out by querying IEDB, but you can't get it directly.

The other downside is that you don't really know anything about the "similarity" criteria used to assign the reactivity_ref.

Is that better or worse than reactivity_id? 8-)

Or do we just drop this for now?

@bcorrie
Copy link
Contributor Author

bcorrie commented Sep 9, 2024

  • Should we change reactivity_method and reactivity_readout to free text (from an enum)?

I am fine either way. The benefit of keeping it as an enum is that if we are confident about the enums we have, we can just add more enums over time. This means that the method and readout are more comparable in the short term at the expense of having users not being able to classify the method as they really want to. If we had an "other" enum we could give users a formal way of stating that my reactivity method isn't any of the ones we have in the standard???

@bcorrie
Copy link
Contributor Author

bcorrie commented Sep 9, 2024

2. Should we leave in Reactivity.reactivity_ref

Yes, but as listed currently:

        reactivity_ref:
            type: array
            description: Array of cross references to external receptor/reactivity records 
            title: Receptor/reactivity cross-references
            items:
                type: string
            example: ["IEDB_RECEPTOR:10"]
            x-airr:
                nullable: true
                adc-query-support: true

I think this should be pointing to an external epitope record, not an external receptor record, no?

For example, if the Reactivity record in question is for the epitope QKRGIVEQCCTSICS should the reactivity_ref not point to IEDB_EPITOPE:1616345 (https://www.iedb.org/epitope/1616345). The AIRR Reactivity object defines the epitope/antigen/species so it seems to make sense this should link to the IEDB epitope not the IEDB receptor?

Probably a cut and paste error?

The AIRR Receptor object defines a receptor and points to the IEDB receptor object as it should.

Am I crazy?

@bcorrie
Copy link
Contributor Author

bcorrie commented Sep 22, 2024

Updated reactivity_method and reactivity_readout to be strings with description listing the recommended field values.

Changed Reactivity.reactivity_ref to point to an IEDB Epitope record rather than a IEDB Receptor record.

Cleaned up the examples.

@bcorrie
Copy link
Contributor Author

bcorrie commented Sep 22, 2024

I think the only remaining question is whether we have:

  • Rearrangement.reactivity_id (ID of an AIRR Reactivity record)
  • Rearrangement.reactivity_ref (CURIE that resolved to external entity e.g. IEDB_RECEPTOR:182992)
  • Comma separated list of one of the above
  • Neither (no change to Rearrangement)

I think I would lean towards having a comma separated list (like v_call) of Rearrangement.reactivity_ref with the spec saying they should be CURIEs that resolve to externally defined entities.

@bcorrie
Copy link
Contributor Author

bcorrie commented Oct 7, 2024

I think I would lean towards having a comma separated list (like v_call) of Rearrangement.reactivity_ref with the spec saying they should be CURIEs that resolve to externally defined entities.

Note that using a CURIE does not preclude referring to an AIRR reactivity record in the ADC in the sense we could in the future define a CURIE for an ADC Reactivity record (e.g: ADC_REACTIVITY:5ed6859e99011334ac05e847).

@bcorrie
Copy link
Contributor Author

bcorrie commented Oct 7, 2024

Have both fields Rearrangement.reactivity_id and Rearrangement.reactivity_ref (CURIE that resolved to external entity e.g. IEDB_RECEPTOR:182992) as comma separated lists.

Order and length of list in one is independent of order and length in the other - need to document this.

@javh javh requested review from javh and bussec October 7, 2024 18:38
@bcorrie
Copy link
Contributor Author

bcorrie commented Oct 7, 2024

Fix "UniProt" camel case to UNIPROT in CURIE.

@bcorrie
Copy link
Contributor Author

bcorrie commented Oct 16, 2024

I believe these changes are done.

@bussec if you can do a quick check that would be good. In particular I removed the IEDB provider, as it was actually not correct. It included only the TCR search, where the JSON response requires both a TCR search and a BCR search to be complete. I don't think there is a way to do this in the spec is there? So I removed the provider in its entirety.

Note the CURIE resolution for IEDB_RECEPTOR and IEDB_EPITOPE are fine, but the JSON API query is the part that is incomplete. To completely query for a Receptor you need to:

                url: "https://query-api.iedb.org/tcr_search?receptor_group_id=eq.{local_id}"
                url: "https://query-api.iedb.org/bcr_search?receptor_group_id=eq.{local_id}"

There is no receptor_search equivalent that spans both bcr and tcr as far as I know.

See commit: 71687d9

@bcorrie
Copy link
Contributor Author

bcorrie commented Oct 18, 2024

@bussec any comments on the IEDB curie question above? If not, I will merge and close...

@javh
Copy link
Contributor

javh commented Nov 4, 2024

From the call:

  • Merge after adding BCR/TCR urls.

@bcorrie
Copy link
Contributor Author

bcorrie commented Nov 7, 2024

Damn, didn't realize we had to have openapi 3 spec synced in language directories - went around in circles for a while...

@bcorrie bcorrie removed the request for review from bussec November 7, 2024 18:05
@bcorrie
Copy link
Contributor Author

bcorrie commented Nov 7, 2024

Done, created a new issue to review the url field in InformationProvider - #811

@bcorrie bcorrie merged commit 70f7a00 into master Nov 7, 2024
8 checks passed
@schristley
Copy link
Member

Damn, didn't realize we had to have openapi 3 spec synced in language directories - went around in circles for a while...

@bcorrie There is a Makefile now at the top level, do make spec-copy to make your life easier! Likewise with make data-copy if you are updating test data, which is now in a central location and consistent across the languages.

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.

How do I capture known antigen/epitope reactivity to AIRR objects (Rearrangement/Cell)
3 participants