cleaner deeptable instantiation pattern

[bmschmidt]

Testing scaling to 10 billion points.

---

Important

Introduces fromQuadfeather method in Deeptable.ts for cleaner instantiation with quadfeather data and updates scatterplot.ts to use this method.

• Deeptable Instantiation:
  • Introduces fromQuadfeather static method in Deeptable.ts for initializing Deeptable with quadfeather data.
  • Handles asynchronous loading of tile manifests and initializes Deeptable with loaded data.
• Function Refactoring:
  • Refactors tileManifest to loadTileManifest in Deeptable.ts for better clarity.
• Scatterplot Updates:
  • Updates scatterplot.ts to use fromQuadfeather method for Deeptable instantiation, replacing from_quadfeather.

^{This description was created by for 5703fd8. You can customize this summary. It will automatically update as commits are pushed.}

[bmschmidt]

• refactor manifests to be lazier #173 : 2 dependent PRs (#174 , #195 )
• uv for quadfeather #154
• cleaner deeptable instantiation pattern #172 👈 (View in Graphite)
• update evaluation set size on sortedDataSelection #192 : 1 other dependent PR (#193 )
• no linting in ci #194 : 1 other dependent PR (#81 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 89b9a63 in 1 minute and 51 seconds. Click for details.

• Reviewed 92 lines of code in 2 files
• Skipped 1 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/Deeptable.ts:191

• Draft comment:
  Consider replacing the lazy manifest check (using '!tileManifest.max_ix') with an explicit flag for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 70% The comment makes a reasonable suggestion - using a property presence check as a type discriminator is a bit hacky. An explicit flag would make the code's intent clearer. However, this seems like a relatively minor improvement and the current code is not obviously wrong or problematic. The TODO comment already acknowledges this could be cleaner. The comment may be premature optimization - the current code works and the meaning is documented in the TODO. Without more context about the manifest types and their usage patterns, it's hard to be certain this change would be an improvement. While an explicit flag would be cleaner, the current code is functional and its intent is documented. The suggested change would be nice-to-have but not critical. The comment makes a reasonable suggestion but is not critical enough to keep, given the existing TODO and working code.

2. src/Deeptable.ts:147

• Draft comment:
  Good use of a static async 'fromQuadfeather' method with camelCase; consider adding a comment on tileProxy support for future expansion.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 70% The comment suggests adding a comment for future expansion, which aligns with the rule that extremely complicated code needs comments. However, it doesn't specify if the code is complicated or if the comment is necessary for understanding. It seems more like a suggestion for documentation rather than a necessary code review comment.

3. src/scatterplot.ts:376

• Draft comment:
  Updated to use the new 'fromQuadfeather' API; ensure downstream error handling for unsupported tileProxy cases.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% The comment has two parts: 1) Noting the API change - this is purely informative and not actionable. 2) Suggesting to ensure error handling - this is speculative ("ensure that...") and doesn't point to a specific issue. The comment seems to violate the rules about not making purely informative comments and not asking authors to verify/ensure things. The comment could be pointing to a real error handling issue with tileProxy that I'm missing. The API change could introduce breaking changes that need attention. While those are valid concerns, the comment doesn't provide specific evidence of issues. If there were known error handling problems, they should be called out directly. API changes should be handled through tests and types, not comments. This comment should be deleted as it is purely informative about the API change and makes a speculative suggestion about error handling without identifying a specific issue.

4. src/Deeptable.ts:148

• Draft comment:
  Typographical issue: In the comment, consider changing "version 2.0 or great of the quadfeather package" to "version 2.0 or greater of the quadfeather package".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70% While this is technically correct - it is a typo - the rules state not to make purely informative comments or comments that are obvious/unimportant. A minor typo in a docstring that doesn't affect functionality or clarity is not worth a comment. The meaning is still clear even with "great" instead of "greater". The typo could theoretically cause confusion for non-native English speakers. Documentation quality is important for public APIs. While documentation quality matters, this typo is so minor that it doesn't meaningfully impact understanding. The rules explicitly say not to make purely informative comments. Delete this comment as it violates the rule against making purely informative comments that don't require code changes.

Workflow ID: wflow_FeanLWLUKKo3n9D0

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 5703fd8 in 1 minute and 51 seconds. Click for details.

• Reviewed 103 lines of code in 3 files
• Skipped 1 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/Deeptable.ts:144

• Draft comment:
  Good improvement using _makePromise in the constructor. Note that the lazy manifest check (if (!tileManifest.max_ix)) is still marked TODO—consider refining this check to robustly detect incomplete manifests.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70% The code in question was moved from the constructor into a new protected method _makePromise(), but the logic itself wasn't changed. The TODO comment was already there before. The review comment is just highlighting an existing TODO without providing specific suggestions for improvement. It's more of an observation than an actionable code review comment. The comment does point out a potential robustness issue in the code. Perhaps there could be cases where max_ix exists but the manifest is still incomplete? While the concern about robustness is valid, simply noting an existing TODO without providing concrete suggestions for improvement doesn't help the PR author. The comment doesn't meet our bar for being clearly actionable. This comment should be deleted as it merely points out an existing TODO without providing specific, actionable feedback about changes made in this PR.

2. src/Deeptable.ts:185

• Draft comment:
  Consider rewriting _makePromise to use async/await consistently instead of mixing .then with async. This would improve readability.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

3. src/Deeptable.ts:1248

• Draft comment:
  Renaming the manifest loader to 'loadTileManifest' to follow camelCase is a nice improvement.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 70% None

4. src/scatterplot.ts:375

• Draft comment:
  Changing instantiation to 'await Deeptable.fromQuadfeather({ baseUrl, plot, tileProxy })' ensures the deeptable is fully initialized before use. This async pattern is an improvement over the previous synchronous style.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 70% This comment is purely informative, praising the change without providing any actionable feedback or suggestions. It doesn't align with the rules as it doesn't offer a specific code suggestion or highlight a potential issue.

5. src/scatterplot.ts:368

• Draft comment:
  In the load_deeptable method, handling of multiple data sources (source_url, arrow_table, arrow_buffer) looks solid—just ensure downstream code always awaits the deeptable's readiness to avoid race conditions.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

6. src/Deeptable.ts:148

• Draft comment:
  Typographical note: The comment says "version 2.0 or great"; it likely should read "version 2.0 or greater".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% The rules say not to make purely informative comments and to avoid obvious or unimportant changes. While this is technically about changed code, fixing a typo in a docstring is extremely minor and doesn't affect functionality. The meaning is still clear even with the typo. The typo could theoretically cause confusion for users reading the API docs. Documentation quality is important. While docs are important, this typo is so minor that the meaning is still completely clear. The cost of the PR review comment outweighs the tiny benefit. Delete this comment as it's too minor and doesn't affect functionality. The rules specifically say not to make purely informative comments.

7. src/Deeptable.ts:182

• Draft comment:
  The comment "Internal function to ensure" appears incomplete. Please verify if additional context or wording is intended.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 70% This comment is asking the PR author to verify if additional context or wording is intended, which violates the rule against asking the author to confirm their intention or to ensure the behavior is intended. It does not provide a specific code suggestion or request for a test, nor does it address any of the additional rules provided.

Workflow ID: wflow_xQRsxPWYfhLbdJTg

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Ensure the limitation of throwing on tileProxy in the fromQuadfeather factory method is documented.

Suggested change

	* @param tileProxy A tileProxy to use for fetching tiles. Can be used to wrap authentication.
	* @param tileProxy A tileProxy to use for fetching tiles. Passing this will throw, as it is not yet supported.

[graphite-app]

There appears to be an inconsistency in the fromQuadfeather method. It throws an error when tileProxy is provided with the message 'Not yet supported', but then proceeds to pass this same tileProxy parameter to the Deeptable constructor. This could lead to confusing behavior since the code suggests the parameter isn't supported in one place but attempts to use it in another.

Consider either:

1. Implementing proper support for the tileProxy parameter, or
2. Not passing the tileProxy to the constructor when it's defined

This would ensure consistent behavior and prevent potential issues when a tileProxy is provided.

Suggested change

	if (tileProxy !== undefined) {
	throw new Error('Not yet supported');
	} else {
	if (tileProxy !== undefined) {
	// We'll handle tileProxy properly in a future update
	console.warn('tileProxy parameter is not fully supported yet');
	}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

[bmschmidt]

Merge activity

• May 26, 4:18 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 26, 4:23 PM UTC: Graphite rebased this pull request as part of a merge.
• May 26, 4:24 PM UTC: @bmschmidt merged this pull request with Graphite.