Skip to content

Store agent objectives as a map #543

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

alexdewar
Copy link
Collaborator

Description

As we're only currently supporting the "single" objective type (and likely won't support anything else for the forseeable future) I was able to write this in a more simplified way than planned. I've written a check to disable the other objective types for now.

I'm opening this as draft because this might not be uncontroversial 😛. I haven't added commodity IDs as keys to the map yet (#542) because I wanted to run this by you before building on top of it, plus I think it can wait till later anyway.

Instead of a HashMap<u32, Vec<AgentObjective>>, I've gone with HashMap<u32, ObjectiveType>, as a) we will only have one objective per year and b) that objective won't have any additional parameters. Writing things this way made the validation checks simpler and it should be slightly simpler to use too.

Now the check_agent_objectives function is unused outside of the unit tests. I'd be inclined to delete it with the tests for now (we can leave a note somewhere saying we've done this) -- but it's also probably pretty harmless so we could just leave it.

If you're happy with all this, I was planning on refactoring the tests a bit to use rstest fixtures, like elsewhere.

Closes #506.

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

@alexdewar alexdewar requested a review from Copilot May 9, 2025 14:05
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors agent objectives by changing their representation from a vector to a map keyed by milestone year, simplifying objective lookups and validation logic. Key changes include updating the CSV parsing logic to handle string year representations, refactoring objective validations to populate an AgentObjectiveMap, and adapting related tests and structures accordingly.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/input/agent/objective.rs Updated CSV parsing and validation for agent objectives; replaced vector with map.
src/input/agent/commodity_portion.rs Adjusted tests to construct AgentObjectiveMap instead of a vector.
src/input/agent.rs Updated agent creation and tests to reflect the new objectives representation.
src/agent.rs Revised documentation and type definitions to use AgentObjectiveMap.
Comments suppressed due to low confidence (1)

src/input/agent/objective.rs:83

  • The error message does not properly interpolate the variables 'agent_id' and 'year'. Consider using '{}' placeholders and providing the variables as arguments to ensure the message displays the correct values.
ensure!(agent_objectives.contains_key(year), "Agent {agent_id} is missing objectives for year {year}");

Copy link

codecov bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 78.94737% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.49%. Comparing base (9fda41e) to head (a5de4a0).

Files with missing lines Patch % Lines
src/input/agent/objective.rs 77.41% 4 Missing and 3 partials ⚠️
src/input/agent.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #543      +/-   ##
==========================================
- Coverage   92.84%   92.49%   -0.35%     
==========================================
  Files          37       37              
  Lines        4274     4173     -101     
  Branches     4274     4173     -101     
==========================================
- Hits         3968     3860     -108     
- Misses        149      153       +4     
- Partials      157      160       +3     

☔ 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.

@alexdewar alexdewar requested a review from tsmbland May 9, 2025 14:10
@tsmbland
Copy link
Collaborator

tsmbland commented May 9, 2025

I'm fine with this. Keep the code simple for now. I'd say delete check_agent_objectives and provide a link to this PR in an appropriate place

@tsmbland
Copy link
Collaborator

tsmbland commented May 9, 2025

We just need to make sure we keep in the back of our minds that multiple objectives will be added at some point in the future, and do things in a way that makes that as easy as possible when the time comes

@alexdewar
Copy link
Collaborator Author

I meant to mark this as draft before... anyway, this is ready for review now whenever you are.

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 map for agent_objectives
2 participants