PEP 1 #907
PEP 1 #907
Conversation
|
@par-hermes format |
src/interface/packages.hpp
Outdated
| // Returns a sub-Dictionary containing just pointers to packages of type T. | ||
| // Dictionary is a *new copy*, and members are bare pointers, not shared_ptr. | ||
| template <typename T> | ||
| const std::vector<T*> AllPackagesOfType() const { | ||
| Dictionary<T*> sub_dict; | ||
| for (auto package : packages_) { | ||
| if (T *cast_package = dynamic_cast<T*>(package.second.get())) { | ||
| sub_dict[package.first] = cast_package; | ||
| } | ||
| } | ||
| return sub_dict; | ||
| } | ||
|
|
||
| // Returns a list of pointers to packages of type T. | ||
| // List contains bare pointers, not shared_ptr objects | ||
| template <typename T> | ||
| const std::vector<T*> ListPackagesOfType() const { | ||
| std::vector<T*> sub_list; | ||
| for (auto package : packages_) { | ||
| if (T *cast_package = dynamic_cast<T*>(package.second.get())) { | ||
| sub_list.append(cast_package); | ||
| } | ||
| } | ||
| return sub_list; | ||
| } |
There was a problem hiding this comment.
I don't super object to the new dictionary machinery here, but I wonder if a better approach might be to expose a loop structure. For example:
template <typename T, typename F>
void ForAllPackagesOfType(F &f) const {
for (auto &[name, pkg] : packages_) {
if (T *cast_package = dynamic_cast<T*>(pkg.get())) {
f(name, package);
}
}
}There was a problem hiding this comment.
I considered that, but I think it would be common/useful to add other elements besides just the function call -- e.g. all my loops right now include extra code that can print the function being called, and I can see them as a good point for adding other debugging or common code.
I also sometimes (often) need control of the order that functions are called, e.g. one "floors" function relies on the results of previous. Currently I just hack it by pulling out the package I know needs to be first, then looping over the others. A more general approach might be callback "priorities," but that gets hard to follow reading the code and is more complex to implement so it's not clear we'd want it.
Since only maybe half my callers look anything like that prototype, and I gave up trying to cover the different cases. I'm happy to provide it for other implementations though.
There was a problem hiding this comment.
Nah it shouldn't matter much.
|
@par-hermes format |
|
I've had to additionally mark some |
I think that's fine |
|
@bprather looks like the CI is blocking due to a dependency? Is that right? |
|
There aren't unmet dependencies -- it looks like the check itself is broken ( EDIT upstream issue: gregsdennis/dependencies-action#24 |
|
You are likely the one who would fix it. So I defer: do you want to remove it or try to debug it?
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Ben Prather ***@***.***>
Sent: Wednesday, October 18, 2023 10:04:14 AM
To: parthenon-hpc-lab/parthenon ***@***.***>
Cc: Subscribed ***@***.***>
Subject: [EXTERNAL] Re: [parthenon-hpc-lab/parthenon] PEP 1 (PR #907)
There aren't unmet dependencies -- it looks like the check itself is broken (cannot find file ...). Could force-merge this and wait for a fix to the check, or remove the check entirely with a new PR. Happy to do either (I think I introduced the check in the first place)
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/parthenon-hpc-lab/parthenon/pull/907*issuecomment-1768837877__;Iw!!Bt8fGhp8LhKGRg!DYH8Bkeh_7_34oRVZBpM8lMIGSGixX0Mv_ol1-4-nlmMdYM5de-1id3ShL72nqTaaR534KvCU9xie6f_YSRNYzwI$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AOO5OJ7YI4CF4PYRD3SDC6TX774X5AVCNFSM6AAAAAA2IDYRL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRYHAZTOOBXG4__;!!Bt8fGhp8LhKGRg!DYH8Bkeh_7_34oRVZBpM8lMIGSGixX0Mv_ol1-4-nlmMdYM5de-1id3ShL72nqTaaR534KvCU9xie6f_YStjkj0m$>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
|
Looks like it was fixed upstream already! So, I'm gonna hit go on this. |
This is the minimum viable implementation of PEP 1, #816. Ideally this would include documentation, but I'm really not sure what to write short of an example.
PR Summary
See #816 for a description of why this is cool. tl;dr it takes Parthenon's callback model (e.g., driver calls
Update::FillDerived, which then calls any package's registeredFillDerivedfunction) and allows user extensions (e.g., driver callsMyPackages::ApplyFloorsand that calls a bunch of registeredApplyFloorsfunctions depending on which packages are loaded).See here for an example extension package type.
PR Checklist