Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
virus antibody model #253
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?
virus antibody model #253
Changes from 1 commit
321a851
07b922f
e6492dc
f1b2f7f
de3c224
79a7b06
673828a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 possible to make speed and health kwargs on the init? In my view it is good practice to make all attributes explicitly controllable.
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 made the choice of limiting the parameter to these ones because otherwise it's a lot for the user in my opinion. If I add all of them, it would mean more than 10 parameters to be decided by the user. Do you think I should still add all of the other parameters ? Or are there one or two more that would be good to add and still leave the others hidden (like ko_timeout, sight_range, and a few non-crucial parameters) ?
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.
Fair enough. One option is to turn the constants that are the same for all agents of a given class into class level attributes. This still makes it easy to change them without having to add them all to the signature of the
__init__
.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.
Good idea ! It does allow to delete quite a few lines of code in
model.py
also.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.
again, this should not be needed if everything else is implemented correctly.
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 dont understand what is going on 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.
When an antibody looses a confrontation with a virus (because the virus DNA is not in it's memory yet), the antibody is ko for a few steps. It can't move during this time and I symbolise this by setting the agent's target to itself.
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.
Ok, clear.
This might be implemented using event scheduling, which would result in a modest speedup of runtime.
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.
Both agents have a random walk component. Might it be possible to implement this the same way for both and move it to a new super agent class from which both current agents derive?
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.
It could be a good idea ! At first I thought it could also be used for the
duplicate()
method. However, there are a lot of parameters to take into account and the code is very short inside, so I don't think that it would really help to make it derive from a parent class.So I the end, there is just a few lines for the random movement. Isn't a whole parent class a bit overkill ?
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.
If the choice is between repeated code or a parent class, I prefer the parent class. Of course, in python you could also move the relevant code into a helper function (not really appropriate for random walks).
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.
Alright, will do, thank you for the advice !
On another note, did you have time to check out why step was being called on a removed agent ?
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.
Monday was a banking holiday so I had family obligations. I'll try to get to it today, but no promises unfortunately.
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.
Of course, nothing urgent ! Only if you have time :)