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.

Sign up for GitHub

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

Jump to bottom

Prefix method #329

Merged
merged 10 commits into from
Jul 5, 2024
Merged

Prefix method #329

merged 10 commits into from
Jul 5, 2024

Conversation

SamuelBrand1
Copy link
Collaborator

Simple PR that closes #328

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.18%. Comparing base (26822f4) to head (7a768d7).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #329   +/-   ##
=======================================
  Coverage   93.18%   93.18%           
=======================================
  Files          50       50           
  Lines         528      528           
=======================================
  Hits          492      492           
  Misses         36       36

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

@SamuelBrand1
Copy link
Collaborator Author

Added a prefix for simulate as well.

seabbs
seabbs reviewed Jul 3, 2024
View reviewed changes
Copy link
Contributor

@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.

Looks like it will work. Quite a bit of boiler plate here and its unclear to me right now how we will put these different scenarios in our overall dagger framework to compute efficiently?

@SamuelBrand1
Copy link
Collaborator Author

Looks like it will work. Quite a bit of boiler plate here and its unclear to me right now how we will put these different scenarios in our overall dagger framework to compute efficiently?

Mapping across scenarios activates the @Dagger.spawn macro for making EagerThunks, its essentially adding one layer of mapping to what we have now.

seabbs
seabbs previously approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@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.

Agree with the push back. This is good to go for now. I see its pointing at main but really is built on another PR so will need to be careful when merging in

@SamuelBrand1 SamuelBrand1 requested a review from seabbs July 5, 2024 14:13
@seabbs seabbs enabled auto-merge July 5, 2024 15:20
seabbs
seabbs approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@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.

LGTM

@seabbs seabbs added this pull request to the merge queue Jul 5, 2024
Merged via the queue into main with commit d43f033 Jul 5, 2024
10 checks passed
@seabbs seabbs deleted the prefix-method branch July 5, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seabbs seabbs seabbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A prefix method to simplify dispatch on pipeline type for saved data.
3 participants
@SamuelBrand1 @codecov-commenter @seabbs