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

Attach server to individual before calculating the service time #183

Conversation

MichalisPanayides
Copy link
Contributor

This change in the source code can enable the creation of a custom distribution that is server dependent. Currently when trying to access ind.server from a custom distribution class (that inherits from ciw.dists.Distribution) the outcome is False. That is because the server attaches to the individual after the service time is calculated.

The purpose of this PR is to change the order that the attach_server() method and the get_service_time() method are executed.

Changes to attach_server() method:
   - Remove the line that updates the service_end_date for the server.
      It is removed because we want to do that after get_service_time
      is ran for the individual.

Changes to begin_service_if_possible_accept() method:
   - Call attach_server method before getting indvidual's service time
   - Update server's next_end_service_date at the last line only if the
      number of servers are not infinite. This was initially done in the
      attach_server method.
   - Update docstring
Call attach_server() before get_service_time() and update the server's
next_end_service_date attribute for these methods:
   - begin_interrupted_individuals_service
   - begin_service_if_possible_change_shift
   - begin_service_if_possible_release
@coveralls
Copy link

coveralls commented Dec 10, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling d2ff513 on 11michalis11:attach_server_to_individual_before_generating_service_rate into b3be849 on CiwPython:master.

@MichalisPanayides
Copy link
Contributor Author

MichalisPanayides commented Dec 10, 2021

Hey @geraintpalmer. Let me know what you think about these changes 😄

If helpful I can add a subsection on the documentation for server dependent distributions (Probably here)

@geraintpalmer
Copy link
Member

Hi @11michalis11 this looks good, I'm happy with the change.

An example in the documentation about server dependent distributions would be really nice. I think it would be better suited here: https://ciw.readthedocs.io/en/latest/Guides/behaviour/index.html Could you write up an example?

I can merge them at the same time then and draft a new release :)

Thanks for the work!

@geraintpalmer geraintpalmer changed the base branch from master to server-dependent December 17, 2021 10:44
@geraintpalmer geraintpalmer merged commit 8ca4910 into CiwPython:server-dependent Dec 17, 2021
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.

3 participants