Skip to content
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

Update OMTypedParser.py to support variable length arrays (:) #147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ericchapman
Copy link

@ericchapman ericchapman commented Sep 22, 2021

Closes #138

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ericchapman ericchapman changed the title Closes #138 Update OMTypedParser.py to support variable length arrays (:) Sep 22, 2021
@sjoelund sjoelund self-requested a review September 22, 2021 20:02
Copy link
Member

@sjoelund sjoelund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the correct fix. Modelica values do not contain : as expressions. And from the linked issue, the problem is that getComponents() is not part of the typed API. Perhaps it could be parsed if it returned ":" instead of :, but that might require changes to OMEdit as well (and would be an issue for https://github.com/OpenModelica/OpenModelica)

@ericchapman
Copy link
Author

ericchapman commented Sep 22, 2021

@sjoelund Sorry for some of the previous responses. Took me a few tries to understand what you were saying.

Yes, agreed that the parser would have been able to handle this if it was returning ":" instead of :. And also, agreed that it would be dangerous to change these calls to return ":" as that would surely break something as the API would be changing.

However I am not sure I understand why having the parser handle : and replace it with ":" so Python can understand it isn't the fix. I am using numerous calls in the API that dissect the compiled structure and making the proposed change fixed everything and there is no danger of breaking something existing because anything currently returning : is already busted since the parser barfs when it sees it. I think you are saying that : isn't a Modelica expression, but expression or not, there are calls in the API that return it and the parser is unable to handle it. I had to do the following to "make it work" on my end and I REALLY want to avoid having this in my code

        # Send the command
        string = self._omc.sendExpression(expression, parsed=False)

        # TODO: Issue https://github.com/OpenModelica/OMPython/issues/138
        #  OMTypedParser doesn't understand ":" when parsing
        string = string.replace("{:}", "{__UNBOUNDED__}")\
            .replace("{:,", "{__UNBOUNDED__,")\
            .replace(",:,", ",__UNBOUNDED__,")\
            .replace(",:}", ",__UNBOUNDED__}")

        if string.startswith("Error"):
            self.raise_error("Expression '{}' returned the error '{}'.".format(expression, string))

        # Manually call the parser and replace the value
        response = OMTypedParser.parseString(string)

and then later

        # Get the components for this model
        for c in ctx.send("getComponents({})".format(name)):
            element_name, instance_name = c[0:2]
            attribute = c[8]

            # TODO: Hack for unbounded dimensions (see below)
            dimensions = []
            for dim in c[11]:
                dimensions.append(dim if dim != "__UNBOUNDED__" else ":")

I basically have to pre-process the string before passing it to the parser and then post-process it later. I would really love to avoid having this in my code permanently (or pointing to my branched fix forever) and this is the only way I am aware to do it

@sjoelund
Copy link
Member

The problem is that the untyped API has many such special cases that are illegal code. We have many such instances and also cases where AST fragments are returned.

This particular case looks like something that is not all too hard to change in the compiler itself. Perhaps even to typed API and in that case OMEdit could get faster performance by skipping a lot parsing and type checking.

I would guess that OMPython.OMParser could parse this as it was implemented to parse anything in the untyped API. It's quite annoying to use though, especially for things that are returned in a structured way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OMTypedParser does not understand variable length arrays
3 participants