airr-community / airr-standards

AIRR Community Data Standards
https://docs.airr-community.org
Creative Commons Attribution 4.0 International
35 stars 23 forks source link

what is the adc-api-optional attribute in x-airr for the extension fields in ADC API? #658

Closed schristley closed 8 months ago

schristley commented 1 year ago

The extension fields in ADC API has adc-api-optional attribute in x-airr. This attribute is not defined in the AIRR extension properties in the documentation. Is it new? What does it mean?

javh commented 1 year ago

adc-api-optional seems entirely redundant with adc-query_support as they always appear in pairs as:

adc-api-optional: false
adc-query_support: true

Is adc-api-optional needed?

Also, the field should be adc-query-support (no underscores).

bcorrie commented 1 year ago

adc-api-optional was intended to capture fields that are part of the ADC API and not part of the spec. See those fun discussions back when...

https://github.com/airr-community/airr-standards/issues/258

I have fixed the:

Also, the field should be adc-query-support (no underscores). problem.

If we want to revisit the adc-api-optional use and its existence, that would definitely be a 2.0 issue. It certainly can't be resolved in the next two weeks.

bcorrie commented 1 year ago

With that said, it looks to me like in airr-schema.yaml, we have a bunch of fields in the Cell object that has a mix of adc-api-optional = true and adc-query-support=false which seems odd to me.

adc-query_support marks fields that the ADC requires repositories to provide a search capability. The fields in Cell that have adc-api-optional = true are not ADC API optional, they are not required to be searchable.

bcorrie commented 1 year ago

I suspect that adc-api-optional=true was added to the spec for Cell and Receptor fields by mistake, these should have been adc-query-support=true

I have made a commit that makes these changes @bussec can you have a look.

https://github.com/airr-community/airr-standards/commit/7e763ff301b397332ce659cd270de27bae4393ef

This seems to make more sense to me, and with these changes there are no longer any adc-api-optional fields in the AIRR spec files, only the ADC API files.

bcorrie commented 1 year ago

@bussec I think if you are OK with the changes to Cell and Receptor objects then we can move this to the ADC v2.0 milestone to consider whether we need adc-api-optional any more.

Basically what this is saying is that Repositories must implement queries for those fields through the ADC API. These make sense to me, as one needs to search on these fields to find data of interest.

schristley commented 1 year ago

adc-api-optional was intended to capture fields that are part of the ADC API and not part of the spec. See those fun discussions back when...

Ah, okay, adding a entry with what you say in the extension table in the docs should be sufficient to close this issue then.

bcorrie commented 1 year ago

Docs updated...

bcorrie commented 1 year ago

Moving this to v2.0 - consider whether we need the adc-api-optional attribute still

bussec commented 1 year ago

@bcorrie I am ok with moving the changes to v2.0, but I am a bit confused, as they now seem to be part of PR #700 with is a v1.5 Milestone (or at least #699 is)...

bcorrie commented 1 year ago

Moving the adc-api-optional to the ADC spec, and changing all adc-api-optional fields to adc-query-support I think we want to be v1.5. I think most of these were incorrect use of adc-api-optional when the Cell and Receptor objects were created.

The discussion that we will have in v2.0 is around getting rid of adc-api-optional all together.

bussec commented 1 year ago

Ahh... ok, fine with me!

bussec commented 1 year ago

Ahh... ok, fine with me!