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 2 commits 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
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.
There was a problem hiding this comment.
Moved both builders into a new experiment_definition/from_dict.py so they sit next to each other, and pulled out the shared _get_variable_modifiers(modifiers) helper you suggested, both _build_modelica_definition and _build_fmu_definition use it now.
| 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.
There was a problem hiding this comment.
There's now a single _build_experiment_definition(info, custom_function, workspace_id, sal, model=None) that takes the full experiment dict, does the workflow check inline (base["model"].get("modelica")), attaches the extensions itself, and returns ValidExperimentDefinitions.
Experiment.get_definition is three lines now, and Model.get_experiment_definitions goes through the same function with assert isinstance(definition, SimpleModelicaExperimentDefinition), as you suggested. Side effect worth noting: it now passes item["experiment"] rather than just base, so saved definitions with extensions no longer lose them.
Move the FMU and Modelica definition builders into a single 'experiment_definition/from_dict' module and expose one '_build_experiment_definition' function that takes the full experiment dict, picks the workflow inline and attaches the extensions, so both workflows go through the same dict -> entity path. 'Model.get_experiment_definitions' now goes through the same function and asserts that a class based definition is returned.
efredriksson-modelon
left a comment
efredriksson-modelon
left a comment
There was a problem hiding this comment.
Looks good 🍎
Just one comment on the public/private naming that would be good to make a choice on for this PR.
| modelon.impact.client.experiment\_definition.from\_dict module | ||
| -------------------------------------------------------------- | ||
|
|
||
| .. automodule:: modelon.impact.client.experiment_definition.from_dict | ||
| :members: | ||
| :undoc-members: | ||
| :show-inheritance: | ||
|
|
There was a problem hiding this comment.
Why are we documenting this module, we don't expect anyone to use it and is really only for internal dict/build helpers?
I would suggest:
- removing this doc
- rename _build_experiment_definition -> build_experiment_definition so we don't import private methods, but add to the doc-string of it that it is an 'internal' XXX.
Alternative we can rename:
- from_dict -> _from_dict to show this is an internal module
- make build_experiment_definition public. This might be more sane and probably how we though with the _initialize_from module, but we should change so methods we import from it are public.
2 participants
This MR adresses the comments in #343