Set n_PCs rather than nPCs in highd_settings#46
Open
bendichter wants to merge 1 commit into
Open
Conversation
highd_settings() wrote its PC count into settings["nPCs"], but the keyword on Rastermap is n_PCs, so the returned dict carried an unused nPCs key alongside the n_PCs value of 200 inherited from default_settings(). Rastermap(**highd_settings()) raised TypeError for the unexpected argument, and callers who dropped the unknown key got 200 PCs rather than the 400 the function intends. The added test checks that each settings helper returns only keys that Rastermap accepts, which is what would have caught this. It compares against the signature rather than fitting, since highd_settings() asks for 3 splits and fitting it would need a dataset large enough to support the resulting node count. Fixes MouseLand#45
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.
Fixes #45.
highd_settings()wrote its PC count intosettings["nPCs"], but the keyword onRastermapisn_PCs. The returned dict therefore carried an unusednPCs: 400alongside then_PCs: 200inherited fromdefault_settings(), soRastermap(**highd_settings())raisedTypeErrorfor the unexpected argument, and anyone who worked around that by dropping the unknown key silently got 200 PCs rather than the 400 the function intends. Renaming the key fixes both.This does change what the helper returns. Callers who were filtering the dict will now get 400 PCs where they previously got 200, which looks like the original intent, but it is a behaviour change rather than a pure no-op and I wanted to flag it.
The Test
The added test checks that each settings helper returns only keys that
Rastermapaccepts. Without the fix it fails withAssertionError: highd_settings returns unknown keys: {'nPCs'}, and it coversdefault_settingsandsequence_settingsat the same time, both of which are already fine.It compares against the signature rather than fitting anything.
highd_settings()asks for 3 splits on 100 clusters, so actually fitting it needs a dataset large enough to support 800 nodes, and on smaller data it fails insidescaled_kmeansfor an unrelated reason that I have written up separately. Checking the keys is what would have caught this bug, and it runs without needing the test dataset.I did not touch the export list.
highd_settingsandsequence_settingsare reachable only asfrom rastermap.rastermap import ..., since__init__.pyexports justdefault_settingsandsettings_info, which is probably why this went unnoticed. Whether those helpers should be part of the public API seemed like your call rather than something to fold into a typo fix.Note on Tooling
I used Claude Code to find this and write the patch. The test fails on the current code and passes with the change, so the claim does not rest on trusting the tool. Happy to drop it if a change of this shape is unwelcome.