update evaluation set size on sortedDataSelection

[bmschmidt]

Fixes an omission where the overall selection set size wasn't being carried over into a sorted selection from the original selection

---

Important

Fixes omission in SortedDataSelection.fromSelection() to update evaluationSetSize from original selection.

• Behavior:
  • Fixes omission in SortedDataSelection.fromSelection() to update evaluationSetSize from original selection.
• Functions:
  • Modifies addSort() in SelectionTile to use push() instead of pre-allocating array.
• Misc:
  • Adds type casting in SortedDataSelection.fromSelection() for tileFunction.

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

[bmschmidt]

• update evaluation set size on sortedDataSelection #192 : 2 dependent PRs (#172 , #193 ) 👈 (View in Graphite)
• 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 everything up to 1f29cc2 in 1 minute and 42 seconds. Click for details.

• Reviewed 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 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:1067

• Draft comment:
  Ensure consistency: here you sum t.tile.metadata.nPoints while elsewhere (e.g. in wrapWithSelectionMetadata) you use tile.manifest.nPoints. Verify these properties are synonymous, or use one consistently.
• 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% Looking at the code, this is about a change that was made in the diff - adding evaluationSetSize calculation. The comment points out a real inconsistency in property access patterns. However, I don't see strong evidence that these properties are actually different or that using one vs the other would cause issues. Without seeing the Tile class implementation, I can't be certain if metadata.nPoints and manifest.nPoints are truly synonymous. I may be underestimating the importance of consistent property access patterns. Inconsistent patterns could make the code harder to maintain and understand. While consistency is good, without clear evidence that these properties are different or that using one vs the other would cause problems, this comment feels more like a style preference than a necessary change. The comment should be deleted because it's speculative - we don't have strong evidence that the properties are different or that the inconsistency causes any real issues.

Workflow ID: wflow_kXM4keaAlUISNYnp

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 128eff0 in 1 minute and 26 seconds. Click for details.

• Reviewed 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 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:1067

• Draft comment:
  Ensure that each tile’s manifest is populated before summing its nPoints. If not, consider awaiting tile.populateManifest() or adding a guard to avoid runtime errors.
• 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 code already ensures the manifest is populated by awaiting Promise.all() on line 1060 which includes tile.populateManifest(). This means by the time we reach line 1068, the manifest will definitely be populated. The comment is suggesting something that's already been handled correctly in the code. I could be wrong about whether populateManifest() is actually called in that Promise.all() - I'm making an assumption about what get_column() does. Even if I'm wrong about get_column(), the code shows that the author clearly knows about manifest population since they use it elsewhere. The comment is still not providing value. The comment should be deleted since it's suggesting something that's either already handled or that the author is clearly aware of, given the codebase context.

Workflow ID: wflow_msUQDDC4vBXsgbxo

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

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 4637f47 in 1 minute and 14 seconds. Click for details.

• Reviewed 37 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 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:357

• Draft comment:
  Using push to build the pairs array is clearer than preallocating with new Array().
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 70% This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that using push is clearer than preallocating with new Array(), but does not suggest any changes or improvements to the code.

2. src/selection.ts:1049

• Draft comment:
  Await the result of get_column before casting to Vector to ensure the column is resolved.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. src/selection.ts:1068

• Draft comment:
  Set evaluationSetSize by summing tile.manifest.nPoints so that the overall evaluation count carries over to a sorted selection.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_KsvAZmCRFZwaStGW

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

[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:19 PM UTC: Graphite rebased this pull request as part of a merge.
• May 26, 4:21 PM UTC: @bmschmidt merged this pull request with Graphite.