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

rolling out a pipeline type system for dispatching on analysis details #224

Conversation

SamuelBrand1
Copy link
Collaborator

This PR implements dispatching on the type of pipeline <: AbstractEpiAwarePipeline to implement pipeline workflow.

I have kept default constructors and in each case the pipeline-dependent constructor function just returns these. However, there is now a obvious place to extend the pipeline type tree to specify additional behaviour by writing methods for any of the component parts, e.g. make_inference_method(pipeline) etc.

Additionally, the AnalysisPipeline and its unit tests have been refactored into a logical file organisation (obviously opinions make vary) and other minor fixes (e.g. rmprocs has been added to pipeline script).

NB: With reference to #221 and also following f2f meetings this PR is keeping the computation DAG as generated by Dagger.@spawn macros on function calls. This limits the pipeline to any backend which can interface with Julia Base.Distributed, and may be changed in near future.

Closes #218 .

@SamuelBrand1 SamuelBrand1 requested a review from seabbs May 16, 2024 18:35
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.73%. Comparing base (576e0ee) to head (eed267a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #224   +/-   ##
=======================================
  Coverage   92.73%   92.73%           
=======================================
  Files          42       42           
  Lines         399      399           
=======================================
  Hits          370      370           
  Misses         29       29           

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

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.

The overall organisation and high level stuff is good. I don't understand the choice to keep the default functions as it appears to lead to duplication and reduced modularity with no clear upside but maybe I am missing something

pipeline/src/AnalysisPipeline.jl Outdated Show resolved Hide resolved
pipeline/src/constructors/make_inference_method.jl Outdated Show resolved Hide resolved
pipeline/src/constructors/make_latent_models_names.jl Outdated Show resolved Hide resolved
pipeline/src/constructors/make_tspan.jl Outdated Show resolved Hide resolved
pipeline/src/infer/generate_inference_results.jl Outdated Show resolved Hide resolved
pipeline/src/pipeline/do_inference.jl Outdated Show resolved Hide resolved
@SamuelBrand1
Copy link
Collaborator Author

The overall organisation and high level stuff is good. I don't understand the choice to keep the default functions as it appears to lead to duplication and reduced modularity with no clear upside but maybe I am missing something

Mainly, I quite like having default_... functions, but upon reflection very "dev script"-y and I'm being lazy since I can get the effect with make_...(RtwithoutRenewalPipeline()). I'll do a quick change on this.

@SamuelBrand1 SamuelBrand1 requested a review from seabbs May 17, 2024 11:20
@SamuelBrand1
Copy link
Collaborator Author

SamuelBrand1 commented May 17, 2024

I've resolved the comments around using default_... as I think addressed.

If @seabbs could review:

  • Abstracting the map over inference problems to function
  • set of make_... functions.
  • Solution to wanting both the name of latent models as a condensed string and the model itself, i.e. passing as a vector of name => model Pair objects.

That would be great.

@seabbs seabbs enabled auto-merge (squash) May 17, 2024 12:43
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 is really nice. Only one remaining point about file paths which I think can be its own issue as not critical.

@seabbs seabbs merged commit c980e22 into main May 17, 2024
10 checks passed
@seabbs seabbs deleted the 218-rolling-out-a-pipeline-type-system-for-dispatching-on-analysis-details branch May 17, 2024 12:49
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.

Rolling out a Pipeline type system for dispatching on analysis details
3 participants