-
Notifications
You must be signed in to change notification settings - Fork 69
refactor manifests to be lazier #173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
aac2391
to
0491be5
Compare
0491be5
to
cb409c1
Compare
de57740
to
12fb692
Compare
cb409c1
to
cdf64ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed cdf64ac in 1 minute and 37 seconds. Click for details.
- Reviewed
1088
lines of code in17
files - Skipped
3
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. uv.lock:1
- Draft comment:
Review: The lock file pins Python to >=3.12, which may unnecessarily limit users. Also, quadfeather is pinned to a release candidate version (2.0.0rc1). Confirm these versions are intended and document any required caveats. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/Deeptable.ts:149
- Draft comment:
Typo: In the comment on line 149, consider replacing 'version 2.0 or great' with 'version 2.0 or greater' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/tixrixqid.ts:38
- Draft comment:
Typo suggestion: Consider capitalizing 'chat GPT' to 'ChatGPT' for proper noun consistency. - 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 is about proper capitalization of "ChatGPT" in a code comment. While technically correct, this is an extremely minor stylistic issue that doesn't affect code functionality, readability, or maintainability. The review rules emphasize focusing on substantive code issues and avoiding purely informative or unimportant comments. The capitalization could be considered part of maintaining consistent and professional code quality. Proper naming conventions might extend to comments too. The rules explicitly state to avoid obvious or unimportant comments. This capitalization issue is extremely minor and doesn't impact code quality or understanding. Delete this comment as it's too minor and doesn't align with the goal of focusing on substantive code issues.
4. src/types.ts:107
- Draft comment:
Typo: Consider changing "publically documented" to "publicly documented". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_PIB7qNROi92mtNdN
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
12fb692
to
b9d0d80
Compare
cdf64ac
to
ddc25b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed ddc25b7 in 2 minutes and 40 seconds. Click for details.
- Reviewed
1088
lines of code in17
files - Skipped
3
files when reviewing. - Skipped posting
11
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:138
- Draft comment:
When initializing the flatManifest, the check ‘if (!flatManifest[0])’ is used to decide whether to derive manifest info. It would be clearer to compare explicitly to undefined (e.g. flatManifest[0] === undefined) to avoid potential false‐positives. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 70% The comment has a valid point about being more explicit, but several factors make me lean against keeping it: 1) The type definition already shows flatManifest can contain undefined|null, making the check semantically clear 2) The change wouldn't materially affect behavior 3) The current code follows common JS idioms 4) This is a relatively minor stylistic suggestion rather than a functional improvement. I could be undervaluing the benefits of explicit type checking. Some developers might find the explicit undefined check more readable and maintainable. While explicit checks can be beneficial, in this case the existing code is already clear and type-safe. The suggested change would be a purely stylistic preference without meaningful benefits. This comment should be deleted as it suggests a stylistic change that doesn't materially improve code quality or clarity.
2. src/rendering.ts:190
- Draft comment:
For clarity and potential minor performance gains, consider replacing Math.exp(Math.log(k) * zoom_balance) with Math.pow(k, zoom_balance) when computing the size adjustment. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/selection.ts:904
- Draft comment:
The stringmatcher function implements a trie to match UTF-8 encoded strings. A more detailed inline comment with an explicit example would improve readability, as this logic is quite intricate. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold70%
None
4. src/tixrixqid.ts:115
- Draft comment:
The inverse function tixToZxy uses logarithms to compute the zoom level. Please double‐check that the formula (and flooring) works correctly for edge cases and document any assumptions. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold70%
None
5. src/types.ts:76
- Draft comment:
The TileMetadata extent is stored as a string that is then parsed via JSON.parse. Consider documenting the expected format or, if feasible, using a more type‐safe representation. - Reason this comment was not posted:
Confidence changes required:60%
<= threshold70%
None
6. src/typing.ts:37
- Draft comment:
The utility functions (isTransform, isOpChannel, etc.) look fine; just ensure that these type guards remain up‐to‐date if the DS types change in the future. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold70%
None
7. uv.lock:12
- Draft comment:
The package versions (especially for quadfeather) have been updated. Make sure that downstream users are aware of any API changes associated with these version bumps. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold70%
None
8. src/Deeptable.ts:148
- Draft comment:
Typographical: In the comment, consider changing "version 2.0 or great" to "version 2.0 or greater" for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. src/tests/tix.spec.ts:2
- Draft comment:
Typo Alert: The module path '../tixrixqid' looks unusual. Is it intended to be 'tixrixqid' or could it be a typographical error (e.g., missing or extra characters)? Please double-check. - 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 module name does look unusual, but: 1) This is a new file, so the author deliberately wrote this import 2) The tests are actually using all the imported functions successfully 3) The tests pass specific expectations, suggesting the import path is correct 4) We don't have access to the full codebase to know if this is actually a typo 5) If it was a typo, the build would fail immediately The module name is indeed very unusual and could be a legitimate typo that somehow still compiles. Maybe we should keep the comment to be safe? No - if this was a typo, the tests wouldn't work at all. The fact that specific test assertions pass shows this is the intended module name. Plus, our rules explicitly say not to comment on things that would be caught by the build. Delete this comment. If the import path was wrong, the tests would fail during build. The unusual name appears to be intentional since the tests are working.
10. src/tixrixqid.ts:38
- Draft comment:
Typo: Consider changing 'chat GPT' to 'ChatGPT' for proper capitalization. - 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 comment is about capitalization in a code comment, which is extremely minor and doesn't affect code functionality. Our rules focus on code quality, performance, and meaningful changes. Capitalization in comments is not mentioned in any of our rules and doesn't impact code quality or performance. Perhaps consistent capitalization could be considered part of code quality and documentation standards? Maybe the comment helps maintain professional appearance in the codebase? While consistency is good, this is an extremely minor stylistic issue in a comment. Our rules emphasize focusing on meaningful code changes and avoiding purely informative or unimportant comments. Delete this comment as it addresses an extremely minor stylistic issue in a comment that doesn't impact code quality or functionality.
11. src/types.ts:107
- Draft comment:
Typo: "publically" should be "publicly". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_U1q3NbKN4IcTZ1JF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
b9d0d80
to
1aa0ac1
Compare
ddc25b7
to
f7580eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed f7580eb in 1 minute and 17 seconds. Click for details.
- Reviewed
1088
lines of code in17
files - Skipped
3
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/selection.ts:256
- Draft comment:
Bug: In the Bitmask.get() method the code incorrectly uses the index (byte) rather than the value from the mask. Replace: return ((1 << bit) & byte) > 0; with: return ((1 << bit) & this.mask[byte]) > 0; This ensures that the stored bitmask data is properly checked. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/tile.ts:424
- Draft comment:
Improvement: In get_arrow(), the regex used in URL replacement is /.feather/ which matches any character followed by 'feather'. It’s safer to escape the dot as in /.feather/ to ensure a literal match. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/Deeptable.ts:149
- Draft comment:
Consider revising the comment on line 149: "Loads a quadtree created by version 2.0 or great of the quadfeather package..." to "...version 2.0 or greater..." for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/tixrixqid.ts:38
- Draft comment:
Minor typographical note: Consider capitalizing 'chat GPT' to 'ChatGPT' in the comment for clarity and consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold70%
This comment is about a minor stylistic change in a comment, which doesn't impact the functionality or performance of the code. It doesn't align with the rules provided, as it doesn't address any of the specific concerns or suggestions that are allowed. Therefore, it should be removed.
Workflow ID: wflow_LL9Y5pr0FbVpm0lx
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
1aa0ac1
to
d86e17a
Compare
f7580eb
to
903c8a9
Compare
d86e17a
to
12638bd
Compare
903c8a9
to
eec66a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed eec66a0 in 2 minutes and 24 seconds. Click for details.
- Reviewed
1097
lines of code in17
files - Skipped
3
files when reviewing. - Skipped posting
12
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:140
- Draft comment:
The code now uses the new 'metadata' property instead of the old manifest. Verify that all downstream consumers (e.g. get extent, download_to_depth) have been updated to use metadata consistently. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold70%
This comment is asking the PR author to verify that all downstream consumers have been updated to use the newmetadata
property. This falls under the rule of not asking the PR author to confirm or ensure things, which is not allowed. Therefore, this comment should be removed.
2. src/tixrixqid.ts:36
- Draft comment:
The comment about 'chat GPT spat out' is unprofessional. Please remove or rephrase for production quality. - 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 review rules don't specifically mention code comments or professionalism. The comment isn't about code quality, performance, or any of our specific priorities. The existing code comment, while informal, actually provides useful context about the implementation's origin and performance characteristics. Removing it might reduce transparency. The comment could be valid from a professionalism standpoint, and some teams do have strict standards about acknowledging AI tools in code. However, our review rules are focused on functional issues and code quality. Comments about writing style and professionalism are outside our scope unless explicitly mentioned in the rules. This comment should be removed as it's focused on writing style rather than any of our priority areas like performance, code quality, or functionality.
3. src/regl_rendering.ts:240
- Draft comment:
Uniform u_tile_id now uses tile.tix. Ensure that this change is backward compatible with any external code that might reference numeric_id. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold70%
The comment is asking the author to ensure backward compatibility, which is similar to asking them to double-check something. This violates the rule against asking the author to confirm or ensure behavior. The comment does not provide a specific suggestion or point out a specific issue with the code.
4. src/rendering.ts:244
- Draft comment:
In rendering.ts, a for‐loop iterates over all_tiles to call child.populateManifest() asynchronously without awaiting. This may be acceptable for lazy loading but consider adding a comment to justify the non‐blocking behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 70% The non-blocking behavior appears intentional given the use of 'void' keyword. This is likely for performance reasons - we don't want to block the main thread while loading metadata. The method returns visible tiles immediately while triggering background loading. This aligns with the performance requirements of sub-30ms operations and avoiding main thread blocking. I could be wrong about the performance implications. Maybe there are race conditions or other issues with not awaiting these promises. The code shows clear intent with 'void' keyword, and the pattern matches the requirement to avoid blocking the main thread. The behavior appears correct for lazy loading. Delete the comment. The non-blocking behavior is intentional and aligns with performance requirements. The code is clear enough without additional documentation.
5. src/selection.ts:1068
- Draft comment:
Updated selection code now uses tile.metadata.nPoints. Verify that all selection‐size and evaluation metrics continue to work correctly with the new metadata structure. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold70%
This comment is asking the PR author to verify that the selection-size and evaluation metrics work correctly with the new metadata structure. This falls under the rule of not asking the PR author to ensure the behavior is intended or to double-check things. Therefore, this comment should be removed.
6. src/tile.ts:398
- Draft comment:
The get_arrow method now throws if (suffix === undefined). Ensure that this change is propagated to all call sites and that passing null intentions are handled correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold70%
The comment is asking the author to ensure that a change is propagated and that intentions are handled correctly, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or ask for a specific test to be written.
7. src/types.ts:70
- Draft comment:
The TileMetadata 'extent' field is now a string which is later parsed. Consider documenting this conversion requirement, as it may be error‐prone if the format changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 70% This is a significant type change from Rectangle to string, which could introduce bugs if the string format isn't well understood. However, the comment is somewhat speculative ("may be error-prone") and is asking for documentation rather than pointing out a specific issue. The rules say not to make speculative comments or ask for documentation updates. The comment identifies a real potential issue with type safety and maintainability. Converting between structured types and strings can be error-prone. While the concern is valid, the comment violates our rules about not making speculative comments or asking for documentation updates. We should only keep comments that require specific code changes. Delete the comment since it is speculative and requests documentation rather than suggesting a specific code change.
8. uv.lock:124
- Draft comment:
The quadfeather version has been updated to 2.0.0rc1. Confirm that all breaking changes in quadfeather 2.0.0rc1 are compatible with deepscatter's lazy manifest logic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold70%
This comment is asking the PR author to confirm compatibility with breaking changes in a dependency update. It violates the rule against asking the author to confirm intentions or ensure compatibility. The comment does not provide a specific suggestion or point out a specific issue, making it unapproved.
9. src/typing.ts:29
- Draft comment:
Type checking functions (isTransform, isOpChannel, etc.) look good. Ensure consistency between these utility type guards and how channels are processed in API calls. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold70%
None
10. src/Deeptable.ts:40
- Draft comment:
Typographical note: The module name 'tixrixqid' looks unusual. Please double-check if 'tixrixqid' is the intended file name or if it might be a misspelling (e.g., 'tixrixid'). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =60%
<= threshold70%
The comment is asking the author to double-check the module name, which violates the rule against asking the author to confirm their intention or to double-check things. However, it does suggest a possible correction, which could be useful if the name is indeed a typo. The comment is somewhat useful because it points out a potential issue with the naming, but it could be rephrased to be more of a suggestion rather than a request to double-check.
11. src/tixrixqid.ts:38
- Draft comment:
Typographical note: Consider using "ChatGPT" (proper capitalization) instead of "chat GPT" for clarity and consistency. - 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% This is an extremely minor stylistic suggestion about capitalization in a comment. It doesn't affect code functionality, readability, or maintainability. None of the priority rules mention comment capitalization standards. The comment itself is just explaining the source/history of the code implementation. Perhaps consistent capitalization could be considered part of code quality and professionalism. Maybe there's a style guide that specifies how to reference external tools. While consistency is good, this is far too minor of an issue to warrant a PR comment. It's about capitalization in an informal implementation note, not about code quality or functionality. Delete this comment as it's an extremely minor stylistic suggestion about capitalization in a comment that doesn't impact code quality or functionality.
12. src/types.ts:107
- Draft comment:
Typo: Consider changing 'publically' to 'publicly' in the comment for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =70%
<= threshold70%
This comment is pointing out a typo in a comment within the code. Correcting typos can improve code readability and maintain consistency, which aligns with the guideline of using descriptive variable and constant names. However, it doesn't directly relate to functionality or performance. Still, it's a minor but valid suggestion for code quality.
Workflow ID: wflow_OAymgkbsyPg544C3
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -699,7 +699,7 @@ export class Scatterplot { | |||
} | |||
// | |||
const needed_keys = neededFieldsToPlot(prefs.encoding); | |||
this.deeptable.root_tile.require_columns( | |||
void this.deeptable.root_tile.require_columns( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know if I like these void
calls - why is it needed?
// Keep a global index of tile numbers. These are used to identify points. | ||
let tile_identifier = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good riddance
const metadata = { | ||
key: this.key, | ||
min_ix: Number(row.min_ix), | ||
max_ix: Number(row.max_ix), | ||
extent: JSON.parse(row.extent as string) as Rectangle, | ||
nPoints: Number(row.nPoints), | ||
children: [] as string[], | ||
}; | ||
for (const child of tixToChildren(tix)) { | ||
if (deeptable.flatManifest[child]) { | ||
metadata.children.push(tixToZxy(child).join('/')); | ||
} | ||
} | ||
|
||
if (isCompleteManifest(manifest)) this.manifest = manifest; | ||
this._partialManifest = manifest; | ||
this._metadata = metadata; | ||
this.highest_known_ix = metadata.max_ix; | ||
} else { | ||
this._metadata = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be worth commenting more as the metadata is constructed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, might be worth explaining more about the manifest vs metadata distinction somewhere in here, and the flat vs non-flat distinction as well
Merge activity
|
298fe3a
to
0a279b2
Compare
Testing scaling to 10 billion points.
Important
Refactor Deepscatter's tile manifest handling to be lazier, improving scalability and performance for large datasets.
Deeptable
andTile
classes to handle tile manifests lazily, improving scalability for large datasets.Deeptable
to useflatManifest
instead of nestedTileManifest
.points()
method fromDeeptable
.Tile
class to useflatManifest
for metadata and child tile creation.createChildren()
method toTile
for lazy child creation.populateManifest()
andderiveManifestInfoFromTileMetadata()
inTile
.tix.spec.ts
to testtixToChildren
,tixToZxy
, andparentTix
functions.quadfeather
dependency inuv.lock
to version2.0.0rc1
.This description was created by
for eec66a0. You can customize this summary. It will automatically update as commits are pushed.