ansys / pyfluent

Pythonic interface to Ansys Fluent
https://fluent.docs.pyansys.com
MIT License
254 stars 42 forks source link

Confusing FluentEnums #3104

Closed seanpearsonuk closed 1 week ago

seanpearsonuk commented 1 month ago

I don't understand the design decisions here. Maybe we can have a discussion.

>>> from ansys.fluent.core import UIMode
>>> mode_1 = UIMode("no_graphics") # due to base class
>>> mode_1
<UIMode.NO_GRAPHICS: ('gr',)>
>>> mode_2 = UIMode("gr")
Traceback (most recent call last):
...
ValueError: The specified value 'gr' is not a supported value of UIMode. The supported values are: 'no_gui_or_graphics', 'no_graphics', 'no_gui', 'hidden_gui', 'gui'.
>>> mode_2 = UIMode(("gr",))
>>> mode_2
<UIMode.NO_GRAPHICS: ('gr',)>
>>> mode_2.str_value()
'no_graphics'
>>> str(mode_2)
'UIMode.NO_GRAPHICS'
>>> repr(mode_2)
"<UIMode.NO_GRAPHICS: ('gr',)>"

@mkundu1 @prmukherj @hpohekar

Please could you give your perspective, thanks.

hpohekar commented 1 month ago

@seanpearsonuk

It should be redefined as follows - We have implemented LaunchMode in similar way.

>>>from enum import Enum
>>>class UIMode(Enum):
    NO_GUI_OR_GRAPHICS = "g"
    NO_GRAPHICS = "gr"
    NO_GUI = "gu"
    HIDDEN_GUI = "hidden"
    GUI = ""

>>>dir(UIMode)
['GUI', 'HIDDEN_GUI', 'NO_GRAPHICS', 'NO_GUI', 'NO_GUI_OR_GRAPHICS', '__class__', '__doc__', '__members__', '__module__']

>>>mode_1 = UIMode.HIDDEN_GUI
>>>mode_1
<UIMode.HIDDEN_GUI: 'hidden'>

>>>mode_1 = UIMode("hidden")
>>>mode_1
<UIMode.HIDDEN_GUI: 'hidden'>

LaunchMode

>>> from ansys.fluent.core.launcher import LaunchMode
>>> dir(LaunchMode)
['CONTAINER', 'PIM', 'SLURM', 'STANDALONE', '__class__', '__doc__', '__members__', '__module__']
>>> LaunchMode.PIM
<LaunchMode.PIM: 2>
>>> m = LaunchMode(2)
>>> m
<LaunchMode.PIM: 2>
>>>

FluentMode needs an update as well.

>>> from ansys.fluent.core import FluentMode           
>>> dir(FluentMode)
['MESHING', 'PURE_MESHING', 'SOLVER', 'SOLVER_ICING', '__class__', '__doc__', '__members__', '__module__']

>>> fm_1 = FluentMode.MESHING
>>> fm_1
<FluentMode.MESHING: (<class 'ansys.fluent.core.session_meshing.Meshing'>, 'meshing')>

>>> fm_2 = FluentMode('meshing')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\hpohekar\AppData\Local\Programs\Python\Python310\lib\enum.py", line 385, in __call__
    return cls.__new__(cls, value)
  File "C:\Users\hpohekar\AppData\Local\Programs\Python\Python310\lib\enum.py", line 710, in __new__
    raise ve_exc
ValueError: 'meshing' is not a valid FluentMode
hpohekar commented 1 month ago

If we want to use multiple enum values correctly then we need to install aenum package.

pip install aenum

>>> from ansys.fluent.core.session_meshing import Meshing
>>> from ansys.fluent.core.session_solver import Solver

>>> from aenum import MultiValueEnum

>>> class FluentMode(MultiValueEnum):
...     MESHING = Meshing, "meshing"
...     SOLVER = Solver, "solver"
...
>>>
>>> dir(FluentMode)
['MESHING', 'SOLVER', '__class__', '__contains__', '__doc__', '__getitem__', '__init_subclass__', '__iter__', '__len__', '__members__', '__module__', '__name__', '__qualname__']
>>>
>>> FluentMode.SOLVER
<FluentMode.SOLVER: <class 'ansys.fluent.core.session_solver.Solver'>>
>>>
>>> mode_1 = FluentMode("solver")
>>> mode_1
<FluentMode.SOLVER: <class 'ansys.fluent.core.session_solver.Solver'>>
>>>
>>> mode_1 = FluentMode(Solver)   
>>> mode_1
<FluentMode.SOLVER: <class 'ansys.fluent.core.session_solver.Solver'>>
>>>
mkundu1 commented 1 month ago

For enums which can be constructed from strings, the enum name in lowercase is the corresponding string value. So the following holds:

UIMode("no_graphics") == UIMode.NO_GRAPHICS

The RHS value is what is passed to Fluent, g or gr are not user-side strings.

seanpearsonuk commented 1 month ago

Yes @mkundu1 I understand all these points. I didn't give much detail above because I thought we might need a discussion about it to get consensus on a final design. What I really wanted to get to was at the very least to eliminate exposure of the Fluent-side strings in that enum. That's one thing that makes this code confusing. Aside from that, is the UIMode("no_graphics") usage obvious at all? Irrespective of the presence/absence of useful documentation, the code should ideally be simple self-documenting, so that the user does not have to read the additional documentation to follow the complex usage. OTOH, the current code is self-documenting something different from reality.

mkundu1 commented 1 month ago

@seanpearsonuk To have the standard syntax for enums:

uses call syntax to return members by value uses index syntax to return members by name

we can separate the implementation details (Fluent values or the session classes) from the enum definition. Enums will be defined as:

class MyEnum(<Enum base class>):
    SYMBOL = <name or number>

The user-side code to construct enums will be MyEnum.SYMBOL, MyEnum["SYMBOL"] or MyEnum(<name or number>).

seanpearsonuk commented 1 month ago

re deprecation of these, I feel that we should simplify deprecation warning messages because complicated help is often going to be insufficient. E.g. telling user to use ui_mode=UIMode.GUI where UIMode being a member of ansys.fluent.core might not be obvious