Address review comments and rewrite bbknn_ts in the component style - #2
Open
rcannood wants to merge 2 commits into
Conversation
* renamed the component to `era_combat_bbknn`, after the ERA system and the two algorithms it combines * `method_types: [embedding, graph]`: the custom kNN graph is evaluated as is * `preferred_normalization: counts`: the method does its own normalisation * pass `--n_pca_components`, `--n_neighbors_per_batch` and `--total_k_neighbors` through to the integration code, they were unused * store a minimal output object instead of the mutated input * updated the reference to the Nature paper and the links to the renamed `google-research/era` repo * `openproblems/base_python:1` and `[hightime, highmem, midcpu]`, in line with the other methods
The graph construction is vectorised per query batch, which gives the exact same distances and connectivities on the test resource -- the per-cell dictionaries only ever held one distance per neighbour, since the batches partition the cells.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Everything in #1, plus a rewrite of the integration code in the OpenProblems component style. The two PRs are alternatives -- take this one if you're fine with the generated code being reorganised, take #1 if you'd rather keep it verbatim.
On top of #1:
## VIASH STARTblock, abatch_aware_knn()helper and a linear body with the usualprint(..., flush=True)steps (279 -> 144 lines)n_obs <= 1,n_comps <= 0), which can't trigger on a benchmark dataset -- a component should fail loudly there rather than emit a placeholder embeddingThe rewrite is output-identical, not just close. Running your original code and this one in the same image on
resources_test/task_batch_integration/cxg_immune_cell_atlas::1.0.0vs rewrite:132562 nonzeros in every run. The second row is purely the docker tag bump (numpy 2.2.6 -> 2.5.2, scanpy 1.10.4 -> 1.12.3), which the old tag forces anyway since it fails
check_configagainst the currentcommon/submodule.viash test src/methods/era_combat_bbknn/config.vsh.yamlsucceeds.Note that the branch still needs a merge with
mainbefore it can go in --run_benchmarkhas moved on quite a bit since September.Checklist before requesting a review
I have performed a self-review of my code
Check the correct box. Does this PR contain:
Proposed changes are described in the CHANGELOG.md
CI Tests succeed and look good!