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

Fix missing arguments in inquire #754

Merged
merged 20 commits into from
Dec 8, 2021
Merged

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Dec 1, 2021

Resolve #753 by exposing more /INQUIRE capabilities to PyMAPDL.

  • Refactor /INQUIRE to match MAPDL's argument order.

@germa89 germa89 self-assigned this Dec 1, 2021
tests/test_commands.py Outdated Show resolved Hide resolved
tests/test_commands.py Outdated Show resolved Hide resolved
tests/test_commands.py Outdated Show resolved Hide resolved
tests/test_commands.py Outdated Show resolved Hide resolved
@akaszynski akaszynski changed the title Fix/missing arguments in /inq UI re Fix missing arguments in inquire Dec 1, 2021
@akaszynski
Copy link
Collaborator

Changed title.

Avoid slashes in title. When we merge the commit history we'll see the PR and can look for more details, but it should be (mostly) human readable.

@akaszynski
Copy link
Collaborator

Also, change PR notes, you don't need both:

Fix #753
Close #753

Just include 1. You can even make it "human" readable:

This PR resolves #753 by exposing more /INQUIRE capabilities to PyMAPDL.

@akaszynski
Copy link
Collaborator

Some thoughts...

I think this issue highlights one of the main issues with PyMAPDL: We're trying to bridge the gap between python commands and MAPDL commands and sometimes a command (like inquire) makes more sense as a MAPDL command. The command's first arg is so we can save a string array within MAPDL, something not necessary within PyMAPDL because we can simply work with the output string. For example, we should really be implementing it as:

>>> out = mapdl.inquire('APDLPARM', 'ROUTINE')
>>> out
/tmp/ansys/some_directory

Instead, what we should just use:

>>> mapdl.directory
/tmp/ansys/some_directory

Instead, what we should do here is submit a PR tha changes our API from:

mapdl.prep7()
mapdl.k()
mapdl.inquire(...)

To a new implementation:

For MAPDL "commands"

mapdl.cmd.prep7()
mapdl.cmd.k()
mapdl.cmd.inqure()

then it's clear when we add non MAPDL-like features:

>>> mapdl.paramters.routine
'PREP7'
>>> mapdl.paramters.routine = 'POST1'
>>> mapdl.post_processing.plot_nodal_displacement()

Or new solution methods:

>>> mapdl.solution.static_solution(...)
>>> mapdl.solution.modal_solution(...)

Or new basic attributes:

>>> mapdl.directory
/tmp/ansys/some_directory

This way we can have two APIs, users can choose which one they want to implement.

We would do this as a breaking change with the release of ansys-mapdl-core==0.61.0 (or later)


Issues with a refactor

Alternatively, we keep pymapdl the way it is and introduce a new library that then abstracts MAPDL.

@germa89, come up with a prototype for this if you want to go this approach. The biggest issue with the aforementioned approach is it is a huge breaking change and we might not want to go down that route. Then again, the current implementation is starting to get quite bloated; we should have an API that makes sense.

Perhaps we leave ansys-mapdl-core the way it is, and start moving the higher level attributes and modules to a different library. For example, all the plotting, post-processing, abstraction layer routines into a new ansys-mapdl-<library-name> library. Should there be a pymechanical at some point, I don't want there to be overlap and duplication, so maybe it's ok for us to not refactor this module.


Conclusion

For the time being, let's keep the PyMAPDL commands as close to their original implementations as possible; I'm fine with you changing the order of the inquire args back to their original order. Be aware that this will need to be refactored in several locations in this code base. Consider an intelligent way to raise an error if users attempt to use inquire with FUNC as the first argument. I don't know how many use this method.

@germa89
Copy link
Collaborator Author

germa89 commented Dec 3, 2021

Hi Alex

Thank you for the insightful discussion.

I believe PyMAPDL should have the capability to replicate APDL using python as driver.
Saying that, it is clear that it will be a low-level API, in the sense, there will be one-to-one equivalent commands and basic capabilities to work properly with PyMAPDL (do analysis, write files, read results, etc), but always resembling APDL work style.
I believe having a low-level API is fine, it will help to bridge the gap between APDL and Python, and it will make easier for current APDL user to transition to PyMAPDL. In addition, as currently happens with APDL, it won't have much modifications after a certain point, so it will make easier to develop other libraries on top of that.

In the future, a higher level API should be provided (as you mentioned, the two API view). Whether this should be built as a separate library or as an evolution of PyMAPDL, that remains to be discussed and I believe it will take some time to be decided (many considerations need to be made). In my opinion, using another library makes more sense, since in that way, you can keep PyMAPDL as close as possible to APDL, and another library can be developed for the more pythonic users. If this new library should be a completely new library or it should bring some of the Workbench Mechanical API capabilities is something that needs to be discussed as well.

In the meantime, to keep it as close to APDL, I'm refactoring all the calls to Mapdl.inquire to match the APDL behavior (arguments order).

Thank you again.

@koubaa
Copy link
Contributor

koubaa commented Dec 3, 2021

@akaszynski @germa89 there might be a way to do it without breaking anything

we currently have
mapdl = ansys.mapdl.core.launch_mapdl()

where mapdl is an instance of ansys.mapdl.core.mapdl_grpc.MapdlGrpc [1]

we could add a new
mapdl = ansys.mapdl.core.start_mapdl_application()

where mapdl is an instance of ansys.mapdl.core.MapdlApplication

and the new object has a property mapdl.cmd which has the same API [2] as what currently exists. We could encourage use of start_mapdl_application() and migrate examples, without ever needing to break launch_mapdl.

of course the name start_mapdl_application is a strawman, and you can think more carefully about what to call it.


[1] ansys.mapdl.core.mapdl_grpc.MapdlGrpc is a rather ugly class name. Should we change it?
[2] or nominally the same. Anything that isn't a strict mapdl command could be moved out of the cmd object and into another property of mapdl

@akaszynski
Copy link
Collaborator

[1] ansys.mapdl.core.mapdl_grpc.MapdlGrpc is a rather ugly class name. Should we change it?

It's indeed an ugly name. We generally just expose the ansys.mapdl.core.Mapdl class to the user, which is subclassed according to which API they are using (stdio with pexpect, CORBA, or gRPC). I can't see a way around this other than removing the other classes and just using gRPC as the one class to rule them all (but I like backwards compatibility, so probably not).

[2] or nominally the same. Anything that isn't a strict mapdl command could be moved out of the cmd object and into another property of mapdl

I agree with creating this, or a separate API. Changing the existing API would be a breaking change that I would prefer to avoid. A lower level "command" class that just abstracts the Ansys parametric design language is already a base class, we could just add a light wrapper around that and call it something like ansys.mapdl.core.MapdlApplication (perhaps a strawman name as well).

Copy link
Collaborator

@akaszynski akaszynski 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 good to go. I'm rerunning the CI due to a flaky failure; merge when green and rerun if needed.

@koubaa koubaa merged commit 14e6492 into main Dec 8, 2021
@germa89 germa89 deleted the fix/missing-arguments-in-/INQUIRE branch December 8, 2021 19:06
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.

/INQUIRE capabilities are capped.
3 participants