Skip to content

Conversation

@Yoshanuikabundi
Copy link
Contributor

Description

Prior to this PR, optimization_result_collection.to_records() downloads only the energies, first, and last molecules. It doesn't download the actual single point records, just the molecules. So the forces are not downloaded - they're later automatically downloaded when you hit the record.topology property. Today this manifested for me as the trivial line

assert record.topology is not None

taking 20 seconds - a great deal of time in a loop of a thousand elements, and extremely frustrating as I thought I had already downloaded this data. This PR makes improvements to the signatures and docstrings of the collection.to_records() and OptimizationResultCollection.to_basic_result_collection() methods that should enhance discoverability of this surprising behavior.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Add include argument to to_records() to enable users to select which fields to download (and hint to user that not all fields are downloaded by default!)
  • Revise docstrings for to_records() to better reflect their purpose
  • Add a note to OptimizationResultCollection.to_records() docstring to explain that the user may want to call .to_basic_result_collection() first
  • Revise docstring of to_basic_result_collection() with more details
  • Add type annotation and default value to to_basic_result_collection(driver) argument

Status

  • Ready to go

@codecov
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.54%. Comparing base (c0a0b0a) to head (6374b69).
⚠️ Report is 2 commits behind head on main.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Sweet, thanks @Yoshanuikabundi. I'll merge and release once tests pass!

@j-wags j-wags merged commit fef38a0 into main Oct 2, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants