-
Notifications
You must be signed in to change notification settings - Fork 3
CARCADE Refactor 3: Treat Action (+params and tests) #175
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
base: main
Are you sure you want to change the base?
Conversation
|
cainja
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revisit tests when we address some of the comments. I imagine they will lead to fairly large changes to the tests.
| ArrayList<LocationContainer> locs, | ||
| ArrayList<Location> siteLocs, | ||
| PatchSimulation sim) { | ||
| if (type.equals("graph")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use switch cases here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch + enums please
cainja
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add an example input file/user interface for the treat action in the PR description
| ArrayList<LocationContainer> locs, | ||
| ArrayList<Location> siteLocs, | ||
| PatchSimulation sim) { | ||
| if (type.equals("graph")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch + enums please
| for (Object edgeObj : allEdges) { | ||
| SiteEdge edge = (SiteEdge) edgeObj; | ||
| Bag allEdgeLocs = new Bag(); | ||
| if (Objects.equals(coord, "Hex")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just graphSites.getSpan() rather than casting and specifying the type of the graphsites?
| } | ||
| pruneSite(locs, sim, damage, sitesLat, siteLocs); | ||
| } else { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using an enum means that you don't need this.
| */ | ||
| public void step(SimState simstate) { | ||
| PatchSimulation sim = (PatchSimulation) simstate; | ||
| String type = "null"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly prefer if you didn't do this.
| latPositions = (int) (-3.5 * sampleLoc.getSubcoordinates().size() + 30); | ||
|
|
||
| // Determine type of sites component implemented. | ||
| if (comp instanceof PatchComponentSitesSource) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should all happen at the abstraction level of PatchComponentSites
Review Time: Medium
Summary: Adding
TreatAction class, and associated parameters and tests. TheTreatAction will add CAR T-cell agents of specified dose and ratio next to source points or vasculature