Skip to content

feat: expose physical-sample-level results via the SDK - #98

Merged
chris-price19 merged 2 commits into
mainfrom
feat/expose-physical-sample-results
Aug 16, 2026
Merged

feat: expose physical-sample-level results via the SDK#98
chris-price19 merged 2 commits into
mainfrom
feat/expose-physical-sample-results

Conversation

@chris-price19

Copy link
Copy Markdown
Contributor

What

Adds SDK access to sample-scoped computed timeseries results — the physical_sample_timeseries_results rows the backend already produces per physical sample (rheed_quality, composition_metric). These are the headline "sample result" and were previously unreachable from the SDK.

Why

get_physical_sample(psid, align=True) only re-joins each constituent data item's own curated RHEED series — it never touches the sample-scoped table. Reproducing sample-level analyses (e.g. the BTO combined quality × lateral-uniformity ranking) was blocked on this and required an out-of-band production RDS query. This closes that gap. Read-only; the backend endpoint already exists and is org-scoped like the routes the SDK already calls.

Changes

  • New primary method Client.get_physical_sample_timeseries(psid, *, property_names=None) — hits GET /physical_samples/{id}/timeseries/ and returns a long-form DataFrame (property_name, real_time_seconds, value, result_id, last_updated, generating_dbos_workflow_id). Long (not wide) because distinct properties can carry different axes, so a wide join on real_time_seconds would mis-align them. property_names filters client-side. Per-property constituent_data_ids live in df.attrs.
  • New parse helper timeseries/physical_sample.py::physical_sample_timeseries_to_dataframe() (exported from the timeseries package). Maps JSON nullNaN, validates per-property length, and returns an empty frame for samples with no metrics.
  • Secondary (additive): PhysicalSampleResult.sample_metrics is now populated by get_physical_sample() (new include_sample_metrics=True kwarg to opt out). Fetched resiliently — a 404 leaves it None rather than newly raising, so get_physical_sample's lenient behavior is preserved.
  • Docs: "Sample-Level Results" section in analysis-results.rst.
  • Tests: 12 unit tests (mocked _get: null→NaN, filtering, empty, 404→ClientError, length mismatch, sample_metrics populate/skip) + 1 integration test that probes real BTO samples for rheed_quality and skips gracefully.

Error / edge behavior

  • Non-existent sample (404) → ClientError(status_code=404).
  • Existing sample with no computed metrics → empty DataFrame (no exception).
  • Malformed payload (values/time-axis length mismatch) → ValueError (chosen over a bare assert, which -O strips).

Reviewer notes / deviations

  • unix_times forward-compat: the parser passes a unix_times column through if the payload includes it (the planned backend follow-up). Today's responses omit it, so the default output matches the current documented shape and needs no SDK change when that lands.
  • Backend follow-ups out of scope (tracked separately): serializing unix_times for absolute-time alignment, and a server-side property_names filter param. The SDK ships and works without them.
  • Integration test discovers a sample dynamically rather than hardcoding a prod physical_sample_id, matching the repo's existing skip-gracefully convention.

Verification

  • 12 new unit tests pass; related suites (test_rheed_timeseries, test_align) green; full suite (196 tests) collects cleanly.
  • Pinned ruff v0.9.4 lint + format pass on all changed source.
  • Not yet exercised: the integration test's live assertions (finite rheed_quality over a growth window; trimmed-mean Q spot-check) require running against prod with AS_API_KEY.

🤖 Generated with Claude Code

@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown

Greptile Summary

The PR exposes physical-sample-scoped timeseries through a new long-form DataFrame parser and client method, and attaches those metrics to aggregate sample results by default.

  • Adds parsing, filtering, provenance metadata, and optional future unix-time support.
  • Adds client APIs, result-model storage, documentation, and unit/integration coverage.
  • The default enrichment can make existing sample and project fetches fail when only the new endpoint is unavailable.

Confidence Score: 4/5

The new endpoint access is useful, but the default optional enrichment should not be allowed to abort otherwise successful physical-sample and project fetches.

Existing aggregate calls now depend on an additional endpoint whose non-404 HTTP and transport failures propagate, while the documentation also misstates the successful empty-result representation.

Files Needing Attention: src/atomscale/client.py; docs/guides/analysis-results.rst

Important Files Changed

Filename Overview
src/atomscale/client.py Adds the public fetch method and default aggregate enrichment, but the optional extra request can now abort existing sample and project fetches.
src/atomscale/timeseries/physical_sample.py Adds long-form parsing, filtering, validation, dtypes, and provenance metadata with no established current blocking defect.
src/atomscale/results/group.py Adds a backward-compatible optional sample_metrics attribute to PhysicalSampleResult.
docs/guides/analysis-results.rst Documents sample-level results, but incorrectly says no metrics produce None rather than an empty DataFrame.
tests/test_physical_sample_timeseries.py Covers primary parser and client behavior but does not exercise non-404 failures of the default aggregate enrichment.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Client
    participant Samples as Physical Samples API
    participant Metrics as Sample Metrics API
    Caller->>Client: get_physical_sample(id)
    Client->>Samples: GET physical_samples/
    Samples-->>Client: sample metadata
    Client->>Metrics: "GET physical_samples/{id}/timeseries/"
    alt successful response
        Metrics-->>Client: properties
        Client-->>Caller: PhysicalSampleResult + sample_metrics
    else 404
        Metrics-->>Client: 404
        Client-->>Caller: PhysicalSampleResult + None
    else other HTTP or transport failure
        Metrics--xClient: error
        Client--xCaller: exception
    end
Loading

Fix all with Greploop

Fix All in Claude Code Fix All in Conductor

Prompt To Fix All With AI
### Issue 1
src/atomscale/client.py:1117-1121
**Optional enrichment aborts sample fetches**

When the additional metrics request receives a non-404 HTTP error or transport failure, the error propagates from the default-enabled enrichment, causing an otherwise successful `get_physical_sample` call—and an entire `get_project` aggregation—to fail.

### Issue 2
docs/guides/analysis-results.rst:132
**Empty metrics representation is misstated**

The example says `sample_metrics` is `None` when a sample has no computed metrics, but a successful empty response produces an empty DataFrame; callers following this guidance can incorrectly treat that frame as populated data.

```suggestion
   print(sample.sample_metrics)  # Empty DataFrame if fetched but no metrics exist
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "expose physical sample level results" | Re-trigger Greptile

Comment thread src/atomscale/client.py Outdated
Comment on lines +1117 to +1121
sample_metrics: DataFrame | None = None
if include_sample_metrics:
raw_metrics = self._get(
sub_url=f"physical_samples/{physical_sample_id}/timeseries/"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Optional enrichment aborts sample fetches

When the additional metrics request receives a non-404 HTTP error or transport failure, the error propagates from the default-enabled enrichment, causing an otherwise successful get_physical_sample call—and an entire get_project aggregation—to fail.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/atomscale/client.py
Line: 1117-1121

Comment:
**Optional enrichment aborts sample fetches**

When the additional metrics request receives a non-404 HTTP error or transport failure, the error propagates from the default-enabled enrichment, causing an otherwise successful `get_physical_sample` call—and an entire `get_project` aggregation—to fail.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Conductor

.. code-block:: python

sample = client.get_physical_sample(physical_sample_id)
print(sample.sample_metrics) # None if the sample has no computed metrics

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Empty metrics representation is misstated

The example says sample_metrics is None when a sample has no computed metrics, but a successful empty response produces an empty DataFrame; callers following this guidance can incorrectly treat that frame as populated data.

Suggested change
print(sample.sample_metrics) # None if the sample has no computed metrics
print(sample.sample_metrics) # Empty DataFrame if fetched but no metrics exist
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/guides/analysis-results.rst
Line: 132

Comment:
**Empty metrics representation is misstated**

The example says `sample_metrics` is `None` when a sample has no computed metrics, but a successful empty response produces an empty DataFrame; callers following this guidance can incorrectly treat that frame as populated data.

```suggestion
   print(sample.sample_metrics)  # Empty DataFrame if fetched but no metrics exist
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Conductor

The default-enabled sample_metrics fetch only swallowed 404s; any other HTTP
error (500/502/…) or transport failure propagated, aborting an otherwise
successful get_physical_sample call and cascading through get_project's
per-sample loop. Wrap the enrichment fetch/parse in a try/except for
ClientError and RequestException, warn, and leave sample_metrics=None.
The primary get_physical_sample_timeseries method still raises as before.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@munrojm munrojm added the release:minor Minor release label Aug 15, 2026
@chris-price19
chris-price19 merged commit 0c629c9 into main Aug 16, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:minor Minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants