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

Change scan so that compiler specialisation will be active #83

Merged

Conversation

SamuelBrand1
Copy link
Collaborator

Minor fix of scan to address #82 . Also removed docstring for scan because being actively developed in response to #35 .

Closes #82

@SamuelBrand1 SamuelBrand1 linked an issue Feb 28, 2024 that may be closed by this pull request
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Could you give a bit more on why this is a fix and what the impact on the fix is (the latter only if its not that much trouble). It looks a bit subjective from what I have seen so far.

EpiAware/src/utilities.jl Show resolved Hide resolved
EpiAware/src/utilities.jl Outdated Show resolved Hide resolved
@seabbs
Copy link
Collaborator

seabbs commented Feb 28, 2024

Also I assume you checked that it wasn't already specialising?

Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This looks good to me and reasoning makes sense as to the change. A little more justification would be perfect in this PR but as is is enough for me to sign off.

@seabbs seabbs disabled auto-merge February 28, 2024 11:37
@SamuelBrand1
Copy link
Collaborator Author

SamuelBrand1 commented Feb 28, 2024

In light of discussion here and on #82 I changed scan implementation to take a where F <: AbstractEpiModel argument.

The reason for this change is to

  1. avoid potential problems with specialisation on generic functions (flagged by @seabbs in Activate specialisation for functions with function input #82).
  2. Create safety for the code; the only context we want EpiAware.scan used is as part of the latent infection dynamics.
    • The behaviour of latent infs (e.g. model structure) is now determined by the type of EpiModel.
    • The fixed parameters (e.g. GI) are fields of an immutable EpiModel object.
    • Optionally, a callable/functor for the EpiModel can be defined which defines the dynamics and is used with EpiAware.scan.

Having said the above simple performance testing shows no change between doing this and scan(f::F,...) where {F} or even scan(f,...).

Therefore, point 1. should be considered fairly subjective/based on general principles at this point and point 2. is more persuasive.

@seabbs
Copy link
Collaborator

seabbs commented Feb 28, 2024

Create safety for the code; the only context we want

This is really nice by the way and I agree its persuasive

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.74%. Comparing base (8b9edf9) to head (d676612).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage   97.74%   97.74%           
=======================================
  Files           6        6           
  Lines         133      133           
=======================================
  Hits          130      130           
  Misses          3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seabbs
Copy link
Collaborator

seabbs commented Feb 28, 2024

Having said the above simple performance testing

Its interesting it should make a difference but in practice doesn't really. I wonder if its due to julia docs being a little out of step with improvements that have been made or just this case. Shows I guess that its hard to reason on.

@SamuelBrand1
Copy link
Collaborator Author

Having said the above simple performance testing

Its interesting it should make a difference but in practice doesn't really. I wonder if its due to julia docs being a little out of step with improvements that have been made or just this case. Shows I guess that its hard to reason on.

Yeah, at some point we should do a bit of deeper dive.

@seabbs seabbs merged commit d9c9222 into main Feb 28, 2024
5 checks passed
@seabbs seabbs deleted the 82-activate-specialisation-for-functions-with-function-input branch February 28, 2024 11:43
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.

Activate specialisation for functions with function input
3 participants