Skip to content

Create agent_search_space table#462

Merged
tsmbland merged 8 commits intomainfrom
search_space
Mar 20, 2025
Merged

Create agent_search_space table#462
tsmbland merged 8 commits intomainfrom
search_space

Conversation

@tsmbland
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland commented Mar 19, 2025

Description

This PR breaks out search space into its own table, allowing search space to be defined on a year and commodity basis

I've created the new struct AgentSearchSpace which defines the search space for a given agent/year/commodity process, and I've modified the search_space attribute of Agent to be a Vec<AgentSearchSpace>

I've followed a similar process for reading/validating the data as with several other tables, i.e. AgentSearchSpaceRaw, read_agent_search_space, read_agent_search_space_from_iter

There's some validation (checking the commodity and processes exist, checking the milestone year is valid), but still more to be done after #453

I imagine this could change a lot after #453 and #444, but it works for now

Fixes #456

Type of change

  • Bug fix (non-breaking change to fix an issue)
  • New feature (non-breaking change to add functionality)
  • Refactoring (non-breaking, non-functional change to improve maintainability)
  • Optimization (non-breaking change to speed up the code)
  • Breaking change (whatever its nature)
  • Documentation (improve or add documentation)

Key checklist

  • All tests pass: $ cargo test
  • The documentation builds and looks OK: $ cargo doc

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 83.06452% with 21 lines in your changes missing coverage. Please review.

Project coverage is 95.13%. Comparing base (2a58ffe) to head (48df062).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/input/agent/search_space.rs 85.43% 12 Missing and 3 partials ⚠️
src/input/agent.rs 70.00% 1 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
- Coverage   95.47%   95.13%   -0.34%     
==========================================
  Files          31       32       +1     
  Lines        4658     4732      +74     
  Branches     4658     4732      +74     
==========================================
+ Hits         4447     4502      +55     
- Misses        106      119      +13     
- Partials      105      111       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@tsmbland tsmbland marked this pull request as ready for review March 19, 2025 17:17
@tsmbland tsmbland requested a review from alexdewar March 19, 2025 17:17
Copy link
Copy Markdown
Member

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

I think this is fine for now, but I suspect in future we'll mostly want to find agents' search spaces by looking them up with a year and a commodity ID, in which case some kind of map would probably make more sense (e.g. HashMap<(u32, Rc<str>), SearchSpace).

It surprised me that you weren't getting an error when reading in agent_search_space.csv because it's empty and we decided to disallow empty CSV files way back when. In this case, I guess it might make sense for users to not provide any search spaces, but for things like processes and agents we probably want to make sure that the file's not empty, so it would be worth opening an issue for this, I think. If you want to allow empty/missing files for this case you could add a read_csv_optional function or something (see also #106).

Comment thread src/agent.rs
Comment on lines +54 to +55
/// Unique agent id identifying the agent this search space belongs to
pub agent_id: String,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd be tempted to drop this field because we shouldn't need it (though there are some unneeded process IDs lurking around as well: #443)

@tsmbland
Copy link
Copy Markdown
Collaborator Author

Thanks @alexdewar . Yeah I'm sure the structure of this will change down the line, mostly just concerned about getting the data into the program for now

I'll open up a different PR to deal with empty csv files

Going the keep the agent_id field for now as I'm using it here to create the hashmap of search spaces, but again this will probably change down the line

@tsmbland tsmbland merged commit cd7ea10 into main Mar 20, 2025
7 checks passed
@tsmbland tsmbland deleted the search_space branch March 20, 2025 13:32
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.

Create agent_search_space table

2 participants