ansys / pymapdl

Pythonic interface to MAPDL
https://mapdl.docs.pyansys.com
MIT License
419 stars 118 forks source link

Improve `mapdl.convert_script` #2157

Closed clatapie closed 5 months ago

clatapie commented 1 year ago

After some discussions with @vnamdeo, @germa89 and @RobPasMue, improving the function mapdl.convert_script seems essential to facilitate the VM file conversion process.

This can be done with the following changes :

vnamdeo commented 1 year ago

Thanks @clatapie for this initiative and prioritize this activity. The improvement to the mapdl.convert script is essential. As a user, I am expecting this script should convert MAPDL commands in to PyMAPDL equivalent commands (those are already exposed and documented) apart from above improvements as highlighted. Please do let me know if any help needed. Thanks !

clatapie commented 1 year ago

This issue is linked with #1955.

germa89 commented 1 year ago

This is a very good initiative. Let discuss some of the points:

*COM should in fact return a print() or a Python comment #

There is this flag in the Mapdl class called print_com. If you set it to true during initialization or later in a scrip using:

mapdl.print_com = True

PyMAPDL will print all the mapdl.com (/COM) commands to the screen. Shall we replace the /COM commands for prints? I don't think it is a good idea, because you can use /COM command to print to a different file. For example:

/output, myoutputfile,log
/com, This is being writen in the file myoutputfile.log
/com, but not in the console/terminal.
/ouput,  !return output to terminal

If we implement that change, nothing will be writen to the file.

*GET should be converted into var = mapdl.get("VAR"). The python variable var needs to be the one used in the rest of the file

I fully agree with this approach. It is going to be a challenge to efficiently replace the var in the rest of the file though.

  • VWRITE should be converted into print() involving pandas tables.

For the same reasons as the /COM command, I would not advice this. Ideally, you would want to replace the *VWRITE by open and print commands if your script support it. For the VMs maybe it is a good idea, but surely it won't necessary be a good idea for any PyMAPDL script. Specially for the remote connection, where the python script cannot write directly in the working directory of MAPDL. In that case we can convert from:

! from vm13
/COM
/OUT,vm13,vrt
/COM,------------------- VM13 RESULTS COMPARISON ---------------------
/COM,
/COM,                 |   TARGET   |   Mechanical APDL   |   RATIO
/COM,
*VWRITE,LABEL(1,1),LABEL(1,2),VALUE(1,1),VALUE(1,2),VALUE(1,3)
(1X,A8,A8,'   ',F10.0,'  ',F12.0,'   ',1F15.3)
/COM,-----------------------------------------------------------------
/OUT
FINISH

to:

results = f"""
------------------- VM13 RESULTS COMPARISON ---------------------
   RESULT      |  TARGET     |   Mechanical APDL   |   RATIO
Stress, Y (psi)  {Target_values[0]:.5f}    {stress_y:.5f}       {abs(stress_y/Target_values[0]):.5f}
Stress, Z (psi)  {Target_values[1]:.5f}    {stress_z:.5f}       {abs(stress_z/Target_values[1]):.5f}
-----------------------------------------------------------------
"""
print(results)
with open("vm13.vrt", "w") as fid:
    fid.write(results)
# this file is written in the python working directory (`os.getcwd()` output).
# If you need it in the remote MAPDL instance, you can upload it to the MAPDL
# working directory using:
mapdl.upload("vm13.vrt")

But as it can be seen, not all the scripts might require that conversion. Furthermore, it can be extremely manual (the f-string specially).

  • Sections could directly be added to python files with ################## and ~~~~

I'm fine with that. But what is a section in an MAPDL script? There is not such concept in an APDL script.

Pinging @pmaroneh @mcMunich @mikerife for optional feedback (I know you are busy)

clatapie commented 1 year ago

Thank you for this complete feedback @germa89.

I think that if tables are in a MAPDL script, it will always be implemented that way. So in my opinion, I don't think it's specific to VM files. We could add an option for making this change or not (something like python_table=True) but I think it would be valuable not to only keep it for the VM file conversion. I had a talk with @smedikon, another option would be to create an advanced function for this specific conversion.

I was thinking that the comment for each sections could automatically be added before a /prep7 or another markers

Also, we could add an option to check the result obtained by the conversion of the APDL script. It could be a basic run of the file and returning a warning if the script needs further attention or modifications.

germa89 commented 1 year ago

I think that if tables are in a MAPDL script, it will always be implemented ...

I think we should start from end (python user) to beginning (MAPDL code). What is the natural way a python user will use those tables? How can we convert that MAPDL code so it prints/shows what that user is expecting in the way he/she is expecting?

I was thinking that the comment for each sections could automatically be added before a /prep7 or another markers

I really like this!

clatapie commented 11 months ago

I agree! I usually print a table of results with the following Python code:

import pandas as pd
table = [[4, 5, 5, 70], [4, 5, 5, 70]]
columns = ['a', 'b', 'c', 'd']
index=['row_1', 'row_2']
df = pd.DataFrame(table, columns = columns, index=index)
print(df)

It then renders as follow:

       a  b  c   d
row_1  4  5  5  70
row_2  4  5  5  70

Another option could be to use the tabulate library.

RobPasMue commented 11 months ago

Unless pandas is already a dependency... I'd avoid adding it only for "table formatting" purposes. There are many other approaches, some better than others: https://www.educba.com/python-print-table/

Tabulate is not a bad option either. Using format directly is another option. Up to you. But using pandas only for this is a bit of an overkill 😄

clatapie commented 11 months ago

Thank you for your feedback @RobPasMue. You're right about the dependency issue, I will avoid to use pendas then.

vnamdeo commented 11 months ago

@clatapie and all,

We need to expose all supported PyMAPDL equivalent functions via convert_script: mapdl.run("THICK = 1") mapdl.run("SECT,1,SHELL") mapdl.run("SECD,THICK,1") mapdl.run("FINI") mapdl.run("SFCONTROL,2,,1,1,,1,,,2")

mapdl.get("RF_XT01", "FSUM", "", "ITEM", "FX")---> can we assign it to some variable like below var1 = mapdl.get("RF_XT01", "FSUM", "", "ITEM", "FX")

Will append this list. Thanks !

germa89 commented 5 months ago

@clatapie and all,

We need to expose all supported PyMAPDL equivalent functions via convert_script: mapdl.run("THICK = 1") mapdl.run("SECT,1,SHELL") mapdl.run("SECD,THICK,1") mapdl.run("FINI") mapdl.run("SFCONTROL,2,,1,1,,1,,,2")

mapdl.get("RF_XT01", "FSUM", "", "ITEM", "FX")---> can we assign it to some variable like below var1 = mapdl.get("RF_XT01", "FSUM", "", "ITEM", "FX")

Will append this list. Thanks !

The converted has been improved quite a lot in #2433

Now it renders:

"""Script generated by ansys-mapdl-core version 0.68.dev0"""
from ansys.mapdl.core import launch_mapdl
mapdl = launch_mapdl(loglevel="WARNING", print_com=True, check_parameter_names=False)

mapdl.run("THICK = 1")
mapdl.sectype(1, "SHELL")
mapdl.secdata("THICK", 1)
mapdl.finish()
mapdl.run("SFCONTROL,2,,1,1,,1,,,2")
mapdl.exit()

Regarding mapdl.get, you can do:

var1 = mapdl.get_value("RF_XT01", "FSUM", "", "ITEM", "FX")

Same with mapdl.vget and mapdl.get_array.

germa89 commented 5 months ago

After long time with not much work in the specific issues raised in this PR, I have to comment:

*COM should in fact return a print() or a Python comment #

As metioned, this was already in place with ``mapdl.print_com = True```.

*GET should be converted into var = mapdl.get("VAR"). The python variable var needs to be the one used in the rest of the file

Difficult to do this one, because we will have to do regex to search the file for where this var is used. And there could be edge cases. I don't think we have the man power for this.

*VWRITE should be converted into print() involving pandas tables.

For the VMs might be a good reason. For the rests of the cases, I dont think we should replace *VWRITE. Although I don't really like it... at all!

Sections could directly be added to python files with ################## and ~~~~

We cannot really tell what is a section in an APDL script. Hence I do not recommend to do this.

Hence I'm closing this issue. Unless new info, request or work happens.