ModelicaSystem - remove xml_file as class variable

[syntron]

cleanup handling of the xml file & remove another class variable

preparation for OMCPath (PR #317)

needs also PR #321

[syntron]

@ondras12345 do you have an idea why the last commit is needed to make mypy happy?

on my local checks there was no problem; I do not understand the background of the error in line 1528: n is str but should be int - why is n a str? There was no change in this code ...

[ondras12345]

mypy does not like it when you reuse variable names with different types.

OMPython/OMPython/ModelicaSystem.py

Line 1481 in f958964

for n in nameVal:

Here, n is a str. Its scope is not limited to that for loop, so it still exists when you get here:

OMPython/OMPython/ModelicaSystem.py

Lines 1523 to 1528 in f958964

	(n, m, p, x0, u0, A, B, C, D, stateVars, inputVars, outputVars) = result
	self._linearized_inputs = inputVars
	self._linearized_outputs = outputVars
	self._linearized_states = stateVars
	return LinearizationResult(n, m, p, A, B, C, D, x0, u0, stateVars,
	inputVars, outputVars)

mypy does not know the type of result, so it assumes it is compatible with the previous (inferred) type of n. And when you try to pass that to LinearizationResult, it gives you the

OMPython/ModelicaSystem.py:1527: error: Argument 1 to "LinearizationResult" has incompatible type "str"; expected "int"  [arg-type]

This is my best guess.

It looks like there is now an option in mypy to allow variable redefinition, but I'm not sure if it would help in this case. If you want to try it, add this to pyproject.toml:

[tool.mypy]
allow_redefinition = true

[syntron]

@ondras12345 Thanks for your analyses - I now also see the problem in the usage of n. I do prefere to solve it and not use any special options / allow ambiguity.

However, did you notice, that current master (3e9c55b) does pass mypy without error and commit f958964 from this PR fails? There are no changes in this part of the code!

I plan to update this PR to include PR #321 - it removes the first use of n as variable for the for loop.

OMPython/OMPython/ModelicaSystem.py

Lines 1478 to 1482 in f958964

	if self._has_inputs:
	nameVal = self.getInputs()
	for n in nameVal:
	tupleList = nameVal.get(n)

will be changed to

        if self._inputs:  # if model has input quantities
            for key in self._inputs:
                val = self._inputs[key]

[syntron]

Even with PR #321 this needed some additional checks as the variable value is used with different types (str / int / float):

        with open(file=overrideLinearFile, mode="w", encoding="utf-8") as fh:
            for key, value in self._override_variables.items():
                fh.write(f"{key}={value}\n")
            for key, value in self._linearization_options.items():
                fh.write(f"{key}={value}\n")

and

        if self._inputs:
            for key in self._inputs:
                data = self._inputs[key]
                if data is not None:
                    for value in data:
                        if value[0] < float(self._simulate_options["startTime"]):
                            raise ModelicaSystemError('Input time value is less than simulation startTime')

See commit 3ba84ab

[ondras12345]

However, did you notice, that current master (3e9c55b) does pass mypy without error and commit f958964 from this PR fails? There are no changes in this part of the code!

No idea why this is happening. We specify an exact version of mypy, so it shouldn't be that the new CI run pulled a newer version:

OMPython/.pre-commit-config.yaml

Lines 29 to 30 in 3e9c55b

	- repo: https://github.com/pre-commit/mirrors-mypy.git
	rev: "v1.15.0"

And we use

OMPython/.github/workflows/Test.yml

Lines 38 to 39 in 3e9c55b

	- name: Run pre-commit linters
	run: 'pre-commit run --all-files'

so it should check all files in each run.

[ondras12345]

See commit 3ba84ab

I don't think it's worth doing renames like this just to please mypy. If allow_redefinition = true would silence the warning, I think we should do that instead. Although I haven't tested it in this specific case, I do use this option in some of my personal projects without trouble.

[syntron]

See commit 3ba84ab

I don't think it's worth doing renames like this just to please mypy. If allow_redefinition = true would silence the warning, I think we should do that instead. Although I haven't tested it in this specific case, I do use this option in some of my personal projects without trouble.

The rename is not the only point of the commit / PR #321 - it does change other parts of the code at the same place and the rename is just an additional modification at the same point

[adeas31]

@syntron as we discussed earlier, a lot of changes are done and it is a good idea to make a new OMPython release. Can you please add the milestone to your PR so I know which ones I should review for release 4.0.0

[syntron]

@adeas31 Sorry for the many PRs - I was focused on getting ModelicSystem running in docker and WSL that the other stuff was moved aside. This is currently done and defined in issue #315. Besides that, some smaller (independent) changes exist which are in my fork but do not have at PR at the moment. I will add these also to the issue #315 such that it gives an overview.

Regarding milestones, I will indicate it also at this place. From my side, the release could be done now. The OMCPath / ModelicaSystemDoE stuff could then be defined in a 4.1.0 release. I will check all smaller changes if it would makes sense to include them in a 4.0.0 release and mark them accordingly.

See also issue #254

[adeas31]

Great! I like all the work you are doing.
Please add milestone for all open PRs so manage them accordingly.

[syntron]

@adeas31 I tried to but I'm not allowed to add milestones (or I do not know how). Therefore, I added the markers to the list in issue #315. At the end, all the bigger changes would be for a next release (=> 4.1.0) and for the smaller ones feel free to decide ...