refactor: extract FMU definition builder and simplify get_definition#346
refactor: extract FMU definition builder and simplify get_definition#346abhilash-kumar-nair wants to merge 1 commit into
Conversation
Move FMU experiment construction and extensions building into module-level functions parallel to _build_simple_modelica_experiment_definition, and consolidate expansion_from_dict to accept the full expansion dict
abhilash-kumar-nair
force-pushed
the
akn/fix/refactor-cleanup
branch
from
ad287cf to
d3d63d2
Compare
efredriksson-modelon
left a comment
efredriksson-modelon
left a comment
There was a problem hiding this comment.
I think this is an improvement 🧇
What I would like to avoid is things like the FMU experiment missing expansion data. I think this becomes much simpler if the two builders live close to each other. Or, even is unified to a single function that returns ValidExperimentDefinitions and handle the entire dict -> entity part regardless of workflow.
| simulation_options=analysis.get("simulationOptions", {}), | ||
| simulation_log_level=analysis.get("simulationLogLevel", "WARNING"), | ||
| initialize_from=_resolve_initialize_from(workspace_id, sal, modifiers), | ||
| ).with_modifiers(modifiers=variable_modifiers) |
There was a problem hiding this comment.
This one does not support expansion? I think that it would make sense to have this one and _build_simple_modelica_experiment_definition be defined in the same file as they are so similar? The only difference between them is if you give 'fmu' or 'model' (+ compiler options if model). For example a local helper for
variable_modifiers = {
mod["name"]: get_operator_from_dict(mod)
for mod in base.get("modifiers", {}).get("variables", [])
}
would make sense.
| definition = _build_simple_fmu_experiment_definition( | ||
| base, custom_function, self._workspace_id, self._sal | ||
| ) | ||
| if extensions_data: |
There was a problem hiding this comment.
I think it would make sense if the _build methods took info instead of base so they could add extensions themself.
Could even go so far of just having a single build function that does the _get_workflow check inline and return either definition type. Could have an assert for get_experiment_definitions that FMU is not returned as this should never happen given the API. So type matches SimpleModelicaExperimentDefinition.
2 participants
This MR adresses the comments in #343