Skip to content

feat(prepro): standardized displayNames#6066

Merged
maverbiest merged 20 commits into
mainfrom
update-displayname-creation
Mar 5, 2026
Merged

feat(prepro): standardized displayNames#6066
maverbiest merged 20 commits into
mainfrom
update-displayname-creation

Conversation

@maverbiest

@maverbiest maverbiest commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

adding functionality to build displayNames from specimenCollectorSampleId (or subissionId) and metadata in a more structured way, with the option of extracting an ID if specimenCollectorSampleId is in a recognized format

Manual testing

  • build_display_name is now used to generate displayNames for WNV on this preview. displayNames look as expected and an the same number of WNV sequences come up on the preview as on main's preview
  • I also tested build_display_name on a pathoplexus preview by manually bumping the loculus version to a commit from this branch, where it also worked as expected

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://update-displayname-creati.loculus.org

@claude claude Bot added the preprocessing Issues related to the preprocessing component label Mar 2, 2026
@claude

claude Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #3652 (Include submissionId in displayName), #4089 (Add displayNames for all fields and remove sentenceCase logic)

Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread website/src/config.ts Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
@maverbiest maverbiest force-pushed the update-displayname-creation branch from b878882 to 1953160 Compare March 5, 2026 09:03
@maverbiest maverbiest marked this pull request as ready for review March 5, 2026 09:12
@claude

claude Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @maverbiest's task in 3m 38s —— View job


Code Review

  • Read CLAUDE.md instructions
  • Read changed files
  • Analyze code quality, testing, security
  • Post review feedback

The implementation looks solid overall. The use of extract_regex for identifier extraction is a good choice (reusing existing validated logic), and the INSDC vs direct-submission branching is clear. A few issues worth addressing:

Bug — warnings accumulated inside build_display_name (e.g. "identifier could not be parsed") are silently discarded when concatenate() also produces warnings or errors (lines 1327–1328). The early return drops the warnings list. See inline comment.

Dead codeextract_id_field() at line 1598 is never called; it was superseded by extract_regex but left behind. The docstring on line 1193 still references it.

Minor — using identifier directly in the extract_regex call (line 1288) instead of re-evaluating collector_id or submission_id would be clearer. Also a typo: "concatentate" in the docstring.

Testing — no test for the INSDC happy path (clean identifier with no slashes should pass through unchanged).

@maverbiest maverbiest added the preview Triggers a deployment to argocd label Mar 5, 2026
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/tests/test_metadata_processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/processing_functions.py Outdated
Comment thread preprocessing/nextclade/tests/test_metadata_processing_functions.py Outdated
@maverbiest maverbiest merged commit fe33233 into main Mar 5, 2026
47 checks passed
@maverbiest maverbiest deleted the update-displayname-creation branch March 5, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preprocessing Issues related to the preprocessing component preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants