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

issue with the function "getComponentsTest" of the Scripting library #7594

Closed
felix-marsollier2 opened this issue Jun 21, 2021 · 20 comments
Closed
Assignees

Comments

@felix-marsollier2
Copy link

Description

The function "getComponentsTest" described in the note lastest documentation note returns nothing when called in the command line interpreter of OMEdit of the version 1.16.5 of OpenModelica and under windows 10 using the MSL 3.2.3
However, when using the similar function "getComponents" in the same command line interpreter, it looks to return the right output.
Moreover, when using the function "getComponents", I can't capture the output like: output_list := getComponents(...). I get the following error and I wonder why:
return := getComponents(Modelica.Thermal.HeatTransfer.Examples.TwoMasses)
Error occurred building AST
Syntax Error
[:1:0-1:6:writable] Error: Parser error: Unexpected token near: return (RETURN)

Steps to reproduce

1 - In the Command Line Interpreter, load the Modelica Standard Library V3.2.3 if not automatically loaded
2 - write the statement: getComponentsTest(Modelica.Thermal.HeatTransfer.Examples.TwoMasses)
3 - Write the statement: getComponents(Modelica.Thermal.HeatTransfer.Examples.TwoMasses, , useQuotes = false)
4 - Write the statement: return:=getComponents(Modelica.Thermal.HeatTransfer.Examples.TwoMasses, , useQuotes = false)

Expected behavior

I only want the "sub" class of a given class and its name like getSubClasses(Modelica.Thermal.HeatTransfer.Examples.TwoMasses) => {{Modelica.SIunits.Temperature,T_final}, {Modelica.Thermal.HeatTransfer.Components.HeatCapacitor,mass1},...}. Thus I think that the getComponentsTest function is the most appropriate.

Version and OS

  • OpenModelica Version OpenModelica v1.16.5 (64-bit)
  • Versions of used Modelica libraries: 3.2.3
  • OS: Windows 10, 64 bit
@casella
Copy link
Contributor

casella commented Jun 21, 2021

@felix-marsollier2, version 1.16.5 is quite obsolete now, I'd suggest you to use 1.17.0

I guess you get an error because return is a Modelica language keword, see Section 2.3.3 of the language spec.

@adrpo, I am a bit confused by these API function names. I could not find getComponents in the API guide, and what does getComponentsTest mean? Is it a test function? Does it retrieve test cases for components? Can you shed some light here? Thanks!

@felix-marsollier2
Copy link
Author

felix-marsollier2 commented Jun 21, 2021 via email

@sjoelund
Copy link
Member

getComponentsTest wasn't updated for the new instantiation, it seems. It hasn't been touched since it was added in 2015. getComponents is not typed API and cannot be used to assign the result to variables or do anything useful with it (unless you use ZMQ and manually parse the results).

@casella
Copy link
Contributor

casella commented Jun 22, 2021

The problem is that the documentation does not explain what this API function does. @sjoelund do you know that? Could we add a short explanation? And why the "Test" in the name?

@sjoelund
Copy link
Member

I think it was only used to see if OMEdit could use it. The problem is getComponents uses the Interactive.mo API which cannot be used in OMPython or mos-scripts.

@casella
Copy link
Contributor

casella commented Jun 22, 2021

So we have a test function which is documented and gets used by end-users 2015, while the real function cannot. Hmm.

@adrpo
Copy link
Member

adrpo commented Jun 22, 2021

I think getComponentsTest was added as a test to see if we can have a typed API for getComponents. As far as I know we don't use it anywhere. @adeas31, is this the case? As for the interactive API we don't have documentation for it as it should be internal. We need to redo the entire API properly as currently is a mess.

@adrpo
Copy link
Member

adrpo commented Jun 22, 2021

Note that the documentation happens automatically for all functions in the ModelicaBuiltin.mo file. Maybe we should have annotations on which ones should not show up in the documentation.

@casella
Copy link
Contributor

casella commented Jun 22, 2021

I think getComponentsTest was added as a test to see if we can have a typed API for getComponents. As far as I know we don't use it anywhere.

Who do you mean by "we"? I mean, it's documented in the User's Guide, so people will use it, as @felix-marsollier2's case proves. And I guess we are not supposed to removed it point-blank at some point, lest some of our users get stuck using Stone Age versions of OMC because they found it useful but it's no longer supported :)

If it works, it should be properly documented, explaining what it does. If if doesn't, it should be fixed or removed. If it only does partially, we should explain this in the documentation.

@adeas31, is this the case? As for the interactive API we don't have documentation for it as it should be internal.

Use at your own risk :)

We need to redo the entire API properly as currently is a mess.

I agree, but I'm afraid this wont' happen anytime soon. In the meantime, I'd try to clarify the situation with this specific case, if it is not too complicated.

@casella
Copy link
Contributor

casella commented Jun 22, 2021

Note that the documentation happens automatically for all functions in the ModelicaBuiltin.mo file. Maybe we should have annotations on which ones should not show up in the documentation.

That's one possibility, but we shouldn't remove the ones that have been publicly available and documented for years, unless there are compelling reasons to do so.

@adrpo
Copy link
Member

adrpo commented Jun 22, 2021

Who do you mean by "we"?

By "we" I mean any of our clients: OMEdit, OMNotebook, MDT, OMShell, OMPython, OMJulia, etc.
So I think none of these clients call this function as we would have discovered that it doesn't work anymore.
I'm pretty sure also there is no test for it as we would have seen it broke.

@casella
Copy link
Contributor

casella commented Jun 22, 2021

OK, now I get it, sorry. If it is really broken for good, and it's always been, I'm fine to remove it. Maybe it just doesn't work with the example cited by @felix-marsollier2, which is quite involved, since it uses the Multibody library. In that case, since it's been public for some time, we should rather fix it.

@felix-marsollier2, have you used this function successfully in other cases?

@felix-marsollier2
Copy link
Author

I have tried this function with the example from the Thermal and the Fluid libraries of the MSL and with my own library and nothing worked.
If no functions are currently existing in the scripting API to get the list of component and their name in the class, I can use the solution from @sjoelund using the ZMQ and by parsing the return.
Thanks a lot everbody

@adrpo
Copy link
Member

adrpo commented Jun 23, 2021

@felix-marsollier2 you don't need to do that, I'll fix getComponentsTest as it is documented and we have at least one user.

@adrpo
Copy link
Member

adrpo commented Jun 23, 2021

And I will also add a test for it this time so we don't break it again without knowing.

@casella
Copy link
Contributor

casella commented Jun 23, 2021

Thanks! Please also add one line to the documentation briefly explaining what it does.

adrpo added a commit to adrpo/OpenModelica that referenced this issue Jun 25, 2021
- make getComponentsTest work with new instantiation and -d=nfAPI
- make getComponentsTest work with old instantiation via -d=-newInst
@adrpo
Copy link
Member

adrpo commented Jun 25, 2021

This should now be fixed with PR #7615. Try the build from tomorrow. You need to set some command line options to make it work with the new frontend: -d=nfAPI. You can do that via the scripting: setCommandlineOptions({"-d=nfAPI"}); .
You can also use the old frontend with it via: -d=-newInst but I would advise against that as we will deprecate it soon.

@adrpo adrpo closed this as completed in 8db9243 Jun 25, 2021
@adrpo
Copy link
Member

adrpo commented Jun 26, 2021

Thanks! Please also add one line to the documentation briefly explaining what it does.

Ouch, I forgot about that, will try to add it.

adrpo added a commit to adrpo/OpenModelica that referenced this issue Jun 26, 2021
@adrpo
Copy link
Member

adrpo commented Jun 26, 2021

Some more documentation added in #7616.

@felix-marsollier2
Copy link
Author

It works fine. Thanks the team.

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

No branches or pull requests

4 participants