Conversation
syntron
force-pushed
the
add_ModelicaSystemCmd
branch
4 times, most recently
from
280e219 to
04ccf1d
Compare
could be a simulation which stoped before the final time ...
* define specific exceptions
./OMPython/ModelicaSystem.py:1236:72: E999 SyntaxError: f-string: unmatched '['
syntron
force-pushed
the
add_ModelicaSystemCmd
branch
from
04ccf1d to
4aaf761
Compare
adeas31
left a comment
adeas31
left a comment
There was a problem hiding this comment.
LGTM. Just couple of spelling mistakes. Fix them and I will merge it.
OMPython/ModelicaSystem.py
Outdated
|
|
||
| class ModelicaSystemCmd: | ||
| """ | ||
| Execute a simulation by running the comiled model. |
There was a problem hiding this comment.
Typo. Should be compiled instead of comiled.
OMPython/ModelicaSystem.py
Outdated
|
|
||
| def arg_set(self, key: str, val: str | dict = None) -> None: | ||
| """ | ||
| Set one argument for the executeable model. |
OMPython/ModelicaSystem.py
Outdated
| self._timeout = timeout | ||
| self._args = {} | ||
|
|
||
| def arg_set(self, key: str, val: str | dict = None) -> None: |
There was a problem hiding this comment.
the type hint for val seems incorrect - it does not allow the default value None
There was a problem hiding this comment.
@ondras12345 is this needed if default value is None? I read this up to now as a way to define this option as optional, i.e. if I have an argument mytext which is an optional string, I would write:
myfunc(myteyt: str = None)
Which way do you prefere / use?
There was a problem hiding this comment.
I think the correct way to define it is def myfunc(myteyt: str | dict | None = None): otherwise you will get type-checking warning or error. I am not entirely sure about it. I can't find the relevant documentation of it.
There was a problem hiding this comment.
You can do
from typing import Optional
def myfunc(mytext: Optional[str] = None):
passMypy doesn't like your type hint for arg_set:
OMPython-syntron$ mypy OMPython
[...]
OMPython/ModelicaSystem.py:133: error: Incompatible default for argument "val" (default has type "None", argument has type "str | dict[Any, Any]") [assignment]
And it complains about a lot of other things, too. Maybe we should run mypy in github actions CI.
There was a problem hiding this comment.
Optional[X] is exactly the same as X | None:
Optional = typing.Optional
Optional[X] is equivalent to Union[X, None].
But Optional makes it clear to a human that this is an optional argument.
There was a problem hiding this comment.
I will update this to use Optimal[] syntax
Regarding linter: I'm using the checks in PyCharm and flake8; additionally, I have tested pylint. Up-to-now, I did not even knew about mypy - will check ;-)
The linters I'm using (PyCharm and flake8) did not report any error (besides some minor items and the items deactivated by 'noinspection PyPep8Naming, PyUnresolvedReferences' - I have this defined locally for some classes to get rid of messenges like:
- Unresolved reference 'getContinuous'
- Argument name should be lowercase
Regarding checks for OMPython, I think we should align on a reasonable set of checks and use these; locally, additonal checks can be applied.
There was a problem hiding this comment.
@ondras12345 mypy is an interesting tool! ;-)
I got the mypy messages down to these 14 errors in a local branch. All of these are based on existing variable definitions:
OMTypedParser.py:113: error: Argument 1 has incompatible type "bool"; expected "str" [arg-type]
OMTypedParser.py:114: error: Argument 1 has incompatible type "bool"; expected "str" [arg-type]
OMTypedParser.py:115: error: Argument 1 has incompatible type "None"; expected "str" [arg-type]
ModelicaSystem.py:366: error: Need type annotation for "quantitiesList" (hint: "quantitiesList: list[] = ...") [var-annotated]
ModelicaSystem.py:367: error: Need type annotation for "paramlist" (hint: "paramlist: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:368: error: Need type annotation for "inputlist" (hint: "inputlist: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:369: error: Need type annotation for "outputlist" (hint: "outputlist: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:370: error: Need type annotation for "continuouslist" (hint: "continuouslist: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:371: error: Need type annotation for "simulateOptions" (hint: "simulateOptions: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:372: error: Need type annotation for "overridevariables" (hint: "overridevariables: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:373: error: Need type annotation for "simoptionsoverride" (hint: "simoptionsoverride: dict[, ] = ...") [var-annotated]
ModelicaSystem.py:377: error: Need type annotation for "linearinputs" (hint: "linearinputs: list[] = ...") [var-annotated]
ModelicaSystem.py:378: error: Need type annotation for "linearoutputs" (hint: "linearoutputs: list[] = ...") [var-annotated]
ModelicaSystem.py:379: error: Need type annotation for "linearstates" (hint: "linearstates: list[] = ...") [var-annotated]
Found 14 errors in 2 files (checked 5 source files)
The part related to ModelicaSystemCmd is included in this PR - please check!
There was a problem hiding this comment.
@ondras12345 @adeas31 If you are interested, feel free to check master...syntron:OMPython:mypy for the latest data from my side
3 participants
Add a new class ModelicaSystemCmd which handles the calls to the model executable (simulate() and linearize()). This allows a control of the argumens (simflags; new: simargs) as well as a detailed modification of the override option needed for PR #165
On top of PR #278