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

Example mode and unit tests for pipeline functions. #295

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

SamuelBrand1
Copy link
Collaborator

Does as per title and closes #293

@SamuelBrand1 SamuelBrand1 requested a review from seabbs June 18, 2024 10:21
@SamuelBrand1 SamuelBrand1 linked an issue Jun 18, 2024 that may be closed by this pull request
pipeline/src/pipeline/do_inference.jl Outdated Show resolved Hide resolved
pipeline/src/pipeline/do_truthdata.jl Outdated Show resolved Hide resolved
@SamuelBrand1
Copy link
Collaborator Author

I've moved the dispatch on being the example mode pipeline to the constructor for the list of configutations to loop over. Good shout @seabbs it is cleaner this way.

@SamuelBrand1 SamuelBrand1 requested a review from seabbs June 18, 2024 11:00
@seabbs
Copy link
Collaborator

seabbs commented Jun 18, 2024

See my comment. I thought the solution was just calling the abstract method internally to the new method and then calling rand on that?

@SamuelBrand1
Copy link
Collaborator Author

SamuelBrand1 commented Jun 18, 2024

See my comment. I thought the solution was just calling the abstract method internally to the new method and then calling rand on that?

That was the original pass which I didn't like; I don't want to make a method which internally generates its internal abstract super type. To me that is weird.

for example:

abstract type MyAbstractType end
struct AType <: MyAbstractType
end


function foo(pipe::MyAbstractType)
 ...
 return list
end

function foo(pipe::AType)
 list = foo(get_super_type(pipe))
 return rand(list)
end

@seabbs
Copy link
Collaborator

seabbs commented Jun 18, 2024

What about having a DefaultEpiAwarePipeline that is just the abstract type fall backs and calling that? It just seems like a weird pattern (in the sense of forcing a lot of duplication)?

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.

Nice. Seems like a great solution.

@seabbs seabbs enabled auto-merge June 19, 2024 10:01
@SamuelBrand1
Copy link
Collaborator Author

What about having a DefaultEpiAwarePipeline that is just the abstract type fall backs and calling that? It just seems like a weird pattern (in the sense of forcing a lot of duplication)?

I think a DefaultEpiAwarePipeline is tricky because we want to dispatch within the highest level method (highest level from p.o.v. of the type hierarchy).

What I've just pushed is the creation of an internal _selector method which does the selection from the list of possible scenarios (with default being to return all of them).

Going forwards I think the way we have done the pipe leans into trait-based programming (cf SimpleTraits.jl), but it was a bit of a leap to implement that in this PR.

@seabbs seabbs added this pull request to the merge queue Jun 19, 2024
Merged via the queue into main with commit 18f8aec Jun 19, 2024
10 checks passed
@seabbs seabbs deleted the 293-exampletest-mode-for-pipeline-functions branch June 19, 2024 10:17
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.

Example/test mode for pipeline functions
2 participants