fix(ingest): Small ingest fixes for multi-segmented viruses

[anna-parker]

resolves #

preview URL: https://ingest-fixes.loculus.org/

Summary

Small changes to display:

• do not show alignment states if length is 0, otherwise this gets ugly:
  
• allow option to group segment specific metadata under the same header (oneHeader argument in values.yaml) to reduce busy-ness of page.
• 
• do a hard word-break inside sequence details display to avoid issues like:

Alignment grouping fix:

• We assign segment names using nextclade align. When a segment aligns to a reference nextclade can infer a clade name - we were using this name to assign segment names as in CCHF it was equivalent to the segment name. This is actually an exception and cannot be used in general. Here we change the code to use the actual segment name as defined by the nextclade dataset.

Small grouping changes/ Config changes

• ncbi_update_date: as with taxon_id this is not necessarily the same and cannot be concatenated so we also set it to perSegment and oneHeader.

[theosanderson]

taxon_id is not always shared among all segments: as this is an integer we cannot just concatenate the different fields when they differ. Instead we need to set taxon_id as perSegment and oneHeader.

This doesn't make sense to me. I mean I get that maybe NCBI erroneously has different taxon_id s for different segments that should be grouped, but IMO then we should come up with a consensus taxon ID as part of ingest.

(Actually is there any case in our current set of organisms where we expect the taxon ID to [meaningfully/usefully] vary within an organism, irrespective of segments?)

[corneliusroemer]

Great stuff, nice PR message, makes it easy to review!

A few non-blocking comments:

do not show alignment states if length is 0, otherwise this gets ugly:

We might actually want to state explicitly "segment not included" rather than length: 0 - or even not list the segment at all? This would reduce white space a lot. But probably not worth the complexity at this point.

allow option to group segment specific metadata under the same header (oneHeader argument in values.yaml) to reduce busy-ness of page.

I'm not sure I understand what this does. Do you mean grouping of segment specific metadata of different segments under one header, so NCBI_accession_S, NCBI_accession_M, NCBI_accession_L could all be under the same header?

Great to have more per_segment fields and allow them grouped under same header! I think I got it from context now later on in your post.

This is nice:

[anna-parker]

taxon_id is not always shared among all segments: as this is an integer we cannot just concatenate the different fields when they differ. Instead we need to set taxon_id as perSegment and oneHeader.

This doesn't make sense to me. I mean I get that maybe NCBI erroneously has different taxon_id s for different segments that should be grouped, but IMO then we should come up with a consensus taxon ID as part of ingest.

(Actually is there any case in our current set of organisms where we expect the taxon ID to [meaningfully/usefully] vary within an organism, irrespective of segments?)

This is apparently an influenza "feature" (or bug depending how you look at it)- different clades of different segments would sometimes have different specialized taxon_ids - so in influenza it should be a perSegment field however for CCHF and (I think?) most other viruses this should be the same for all segments... But yes maybe not good to have this as perSegment in our default configs then... I can remove it here and try to overwrite that field for influenza configs

[corneliusroemer]

This is apparently an influenza "feature" (or bug depending how you look at it)- different clades of different segments would sometimes have different specialized taxon_ids - so in influenza it should be a perSegment field however for CCHF and (I think?) most other viruses this should be the same for all segments... But yes maybe not good to have this as perSegment in our default configs then... I can remove it here and try to overwrite that field for influenza configs

I couldn't find an example, at least in CCHF of where we currently group segments with different virus tax ids.

Do you have concrete evidence for influenza where segments of the same virus have different taxon_ids? I might have been wrong when I suggested this and you might have taken my word for it. I'm not confident right now that this can indeed happen.

For CCHF, there are a few non-majority tax ids, but so far they are all only on a single segment:

❯ tsv-filter -H ~/Downloads/details\ \(18\).tsv --not-empty ncbi_virus_tax_id_S --str-not-in-fld ncbi_virus_tax_id_S:"3052518" | tsv-select -H -f ncbi_virus_tax_id_L,ncbi_virus_tax_id_M,ncbi_virus_tax_id_S | tsv-pretty 
ncbi_virus_tax_id_L  ncbi_virus_tax_id_M  ncbi_virus_tax_id_S
                                                       170517
                                                       170517
                                                       170517
                                                       170517
                                                       402373
                                                       402369
                                                       402370
                                                       402371
                                                       402372
                                                       402052

[anna-parker]

This is apparently an influenza "feature" (or bug depending how you look at it)- different clades of different segments would sometimes have different specialized taxon_ids - so in influenza it should be a perSegment field however for CCHF and (I think?) most other viruses this should be the same for all segments... But yes maybe not good to have this as perSegment in our default configs then... I can remove it here and try to overwrite that field for influenza configs

I couldn't find an example, at least in CCHF of where we currently group segments with different virus tax ids.

Do you have concrete evidence for influenza where segments of the same virus have different taxon_ids? I might have been wrong when I suggested this and you might have taken my word for it. I'm not confident right now that this can indeed happen.

For CCHF, there are a few non-majority tax ids, but so far they are all only on a single segment:

❯ tsv-filter -H ~/Downloads/details\ \(18\).tsv --not-empty ncbi_virus_tax_id_S --str-not-in-fld ncbi_virus_tax_id_S:"3052518" | tsv-select -H -f ncbi_virus_tax_id_L,ncbi_virus_tax_id_M,ncbi_virus_tax_id_S | tsv-pretty 
ncbi_virus_tax_id_L  ncbi_virus_tax_id_M  ncbi_virus_tax_id_S
                                                       170517
                                                       170517
                                                       170517
                                                       170517
                                                       402373
                                                       402369
                                                       402370
                                                       402371
                                                       402372
                                                       402052

I definitely saw when I was running locally that the main reason for not grouping was different taxon id and that there were a lot of different taxon Ids - I will try to find an example.

[corneliusroemer]

Woops, E2E fails.

Here's my promised PR to use tsv-utils again, I recommend reading the docs, it's a neat tool!

#2329

[anna-parker]

As this PR now no longer changes how we handle taxon_id I will merge and we can discuss further on slack if setting this to perSegment is actually required in cases such as influenza

[anna-parker]

I was going to merge but realized that cchf is duplicated on the preview - this might be our odd edge -case but I will relaunch and check if this happens again in an hour or so.

[corneliusroemer]

IIUC, setting per segment can be done by downstream users if the need arises. For default organisms it seems unneeded.

…

On Tue, Jul 23, 2024, 17:55 Anna (Anya) Parker ***@***.***> wrote: As this PR now no longer changes how we handle taxon_id I will merge and we can discuss further on slack if setting this to perSegment is actually required in cases such as influenza — Reply to this email directly, view it on GitHub <#2316 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF77AQMAQKT5BCYLHL43PT3ZNZ4HZAVCNFSM6AAAAABLIQZYUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBVGYZDQMRVG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

[anna-parker]

duplication happened again with ebola - but as cchf reingest is working ok I think this is a side effect of: #1777 and will merge this branch