Skip to content
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

adding data to the storage class #24

Merged
merged 52 commits into from
Oct 19, 2023
Merged

Conversation

YonesMussa
Copy link
Contributor

No description provided.

@YonesMussa YonesMussa added Goal: Documentation Improvements or additions to documentation Waiting For Approval Need Approval From Instructors labels Oct 11, 2023
Copy link
Contributor

@mercere99 mercere99 left a comment

Choose a reason for hiding this comment

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

Please don't include the .metals/ or .vscode/ directories in the pull request. These will vary from one person's setup to another and should not be part of the main project. Likewise do not include executables in the pull request -- right now you have both simple and simple.exe included.

Good use of namespace.

AgentData looks like a solid way of tracking agent actions over time. Some notes about it:

  • You do NOT need to make your own DoubleLinkedList class. You can use std::list, but I'd actually recommend just using std::vector instead, which is incredible efficient.
  • I don't think we will need to track a full vector of action_maps since none of the worlds look to be changing actions over time and I can imagine a vector of maps getting huge. One map, set once, should probably do the trick.

While AgentData does PascalCase functions, AgrentReceiver uses snake_case, and DataReceiver has at least one example of camelCase. I think we are going to try to use PascalCase throughout for function names.

Make sure to remove your DoubleLinkedList.hpp when you shift over to a standard library tool.

I recommend moving your source/DataCollection/README.md file into docs/DataCollection.md instead. Since this is high-level documentation, we should keep all of it in one unified place.

In AgentBase, you added a member function called storeActionMap(). This function creates and AgentData object and stores the action_map into it, but then immediately deletes the AgentData as it falls out of scope. I'd recommend changing this to a member function like:

const auto & GetActionMap() const { return action_map; }

This way you can pull the action map out of any agent you need to and store it as needed.

In WorldBase you have a similar issue. In SetAgentReceiver you pass in a COPY of an AgentReceiver and then make a copy of that one the heap using make_shared. Since you never make other copies, this doesn't really need to be a pointer -- you could have just built it as an AgentReceiver object, I think. That said, you never seem to call SetAgentReceiver, so I'm not sure what the goal is.

In CollectData() you explicitly collect data only for agent number 2. Obviously this code needs to be a lot more flexible; in the very least you should pass in the agent ID to collect data on and assert that that agent exists.

More generally, we might want to refactor WorldBase so that we can provide it with a series of functions to run every time the agents are run so that arbitrary modules can tap into this capability. I'll talk about this idea with the whole class, so don't worry about implementing it just yet, but I do think it might make your development easier going forward.

Your changes to simple_main.cpp are just whitespace; given that two spaces are used everywhere else in core we should probably leave he original version as it was.

Overall, good use of naming, Doxygen comments, and general style.

Thanks for adding the appropriate tests!

@YonesMussa
Copy link
Contributor Author

YonesMussa commented Oct 13, 2023

Changes that were requested have been made

@FergusonAJ FergusonAJ changed the base branch from main to development October 19, 2023 19:37
@FergusonAJ FergusonAJ merged commit 4508593 into MSU-CSE491:development Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Goal: Documentation Improvements or additions to documentation Group 2 Merge: READY Topic: Interfaces Waiting For Approval Need Approval From Instructors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants