-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add optical model importer and refactor imported optical materials #1520
Add optical model importer and refactor imported optical materials #1520
Conversation
…idation checks into separate file
Test summary 4 478 files 6 904 suites 15m 34s ⏱️ Results for commit 01087c3. ♻️ This comment has been updated with latest results. |
…y require real_types
… RefactorOpticalMockTests
…tas into RefactorOpticalMockTests
…size_type instead of size_t
… RefactorOpticalMaterials
This looks great @hhollenb! I still have to finish up reviewing, but one thing I'm not totally sure about (and maybe @sethrj can comment) regarding the |
I haven't had a chance to look at this yet, but I think it would be nice to preserve the contiguous mapping of action IDs from optical processes/models; that lets us do an indirection-free lookup of the post-step action from a sampled process. In other words, in the pre-step we're calculating cross sections into an array of P processes, and an interaction selects an index p from that list. If the processes map to sequential action IDs, then we can just add a constant Given all the other work that has to be done in sampling and evaluating, that's probably a small cost, but I think it will be difficult to refactor to using the "less indirection" method if we choose the "easier to implement" method now. |
Right, we would still have contiguous model action IDs either way; I'm wondering whether we need the extra layer of the |
My reasoning for using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we're really in danger of overthinking/overengineering the import/process/model/mfps stuff. Let's spend our meeting today talking about the PR and the import stuff in general.
Some brainstorming thoughts on our discussion about importing data today:
I'm personally partial towards a data-oriented design with strict separation of logic and data layers. Something like: where all the logic on what data to import is done in the diamond importing classes (or specified in the options) and the rectangle classes just do consistency checks and store the data. The Celeritas input data struct can just be a loose collection of input structs that the params use. Params shouldn't rely upon previously built input data, so the input data can just be moved (red arrows). Inputs consumed by param constructors can free or store what they need as necessary. After the importers we can possibly have a user hook to modify / generate more imported data. I like the idea that there's a fairly static input that we don't do internal hidden decisions on, so users can be confident that what they put into Celeritas won't get modified under the hood. (gotta head out real quick but I'll see if I can expand on the idea a bit more tomorrow) |
@hhollenb Sorry I didn't see your comment before the break, but I agree 100%. Let's discuss at our afternoon meeting if you're available? |
A quick draft of what an input data consuming factory could look like: // Concrete class
class ModelFactory final
{
public:
using ImportPhysicsTable = std::vector<ImportPhysicsVector>;
ModelFactory(ImportPhysicsTable mfp_table)
: mfp_table_(std::move(mfp_table))
{
// check table validity
}
void build_mfp_table(MfpBuilder&)
{
// copy the mfp table into the builder
}
private:
ImportPhysicsTable mfp_table_;
};
// Diagnostic / logging data about the process
struct ProcessRecord
{
std::string name;
std::string description;
ImportProcessClass process_class;
};
// Abstract base class
class ProcessFactory
{
public:
using ModelFactoryPair = std::tuple<std::shared_ptr<Model>, ModelFactory>;
// consume process factory list to create model list
static std::tuple<std::vector<std::shared_ptr<Model>>, std::vector<ModelFactory>>
create_model_factory_list(ActionIdIter& iter,
std::vector<std::unique_ptr<ProcessFactory>>&& processes)
{
std::vector<std::shared_ptr<Model>> models;
std::vector<ModelFactory> factories;
for (auto&& proc : processes)
{
auto model_factory_pairs = proc->create_models(iter);
for (auto&& [model, factory] : model_factory_pairs)
{
models.push_back(std::move(model));
factories.push_back(std::move(factory));
}
}
return std::make_pair(std::move(models), std::move(factories));
}
// create a diagnostic record for the process
virtual ProcessRecord initialize_record() const = 0;
// disable copy constructor to ensure we only ever move factories
ProcessFactory(ProcessFactory const&) = delete;
ProcessFactory& operator=(ProcessFactory const&) = delete;
protected:
// override by specific process to create models and their corresponding factories
virtual std::vector<ModelFactoryPair> create_models(ActionRegistry& reg) = 0;
};
class RayleighProcess : public ProcessFactory
{
public:
using SPConstMaterials = std::shared_ptr<MaterialParams const>;
using SPConstCoreMaterials = std::shared_ptr<::celeritas::MaterialParams const>;
struct Input
{
SPConstMaterials materials;
SPConstCoreMaterials core_materials;
std::vector<ImportOpticalRayleigh> imported_rayleigh;
};
RayleighProcess(std::vector<ImportPhysicsVector> mfp_table, Input input)
: mfp_table_(std::move(mfp_table)), input_(std::move(input))
{}
ProcessRecord initialize_record() const final
{
return ProcessRecord{"rayleigh", "rayleigh process desc", ImportProcessClass::rayleigh};
}
protected:
virtual std::vector<ModelFactoryPair> create_models(ActionIdIter& iter) final
{
auto rayleigh_model = std::make_shared<RayleighModel>(*iter++);
/*
* Use Rayleigh MFP calculator to fill missing entries in mfp_table_
* Same logic as in RayleighModel::build_mfp_table
*/
return std::vector<ModelFactoryPair>{
std::make_tuple(std::move(rayleigh_model), ModelFactory{std::move(mfp_table_)})
};
}
private:
std::vector<ImportPhysicsVector> mfp_table_;
Input input_;
}; The The The Constructing As a test, the
Fields can then be moved into their respective factories in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me forever to get back to this. It also looks like there are some previous comments that weren't addressed?
… RefactorOpticalMaterials
…tas into RefactorOpticalMaterials
… RefactorOpticalMaterials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hhollenb ! Sorry it took so long to get through. Maybe we could plan a little work half-day next week to update this to use inp
?
Refactoring imported optical materials
Added
ImportedMaterials
to act as a common storage for material data used by optical models (Rayleigh and WLS). Like withImportedModels
it helps prevent unnecessary copies ofstd::vector<ImportOpticalRayleigh>
andstd::vector<ImportWavelengthShift>
.RayleighModel
was also updated to use anInput
struct to concisely manage material dependencies for building MFP tables.Model Importer
Similar to
phys/ProcessBuilder
, I added theModelImporter
to create models from imported data, as well as provide user build functionality like warn-and-ignore. TheModelBuilder
concrete base class is meant to serve a similar purpose tophys/Process
, and is just meant to build optical models with an action ID.I'm trying to maintain the behavior of
phys/PhysicsParams
where importing processes/models is separate from being built and registered in the action registry, so a layer betweenModelImporter
andoptical/PhysicsParams
is necessary. The input foroptical::PhysicsParams
will look something like:Simple builders are added to the model .cpp files and made accessible through static factory methods. There's some freedom in
determining where the builders live (or even using some type-erased lambdas if we want to be fancy).
N.B.: This loosely depends on #1519 so I'm leaving this as a draft PR, but can be more or less independently reviewed.