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.

Sign up for GitHub

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

Jump to bottom

GUI configuration supporting string based access #414

Merged
merged 3 commits into from
Aug 25, 2024
Merged

GUI configuration supporting string based access #414

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d4c3da
GUI configuration supporting string based access
j8xixo12 Aug 18, 2024
d25d5c2
move Accessor out of GUIConfig and fix docstring style
j8xixo12 Aug 22, 2024
823342a
make class compatible with python 2/3
j8xixo12 Aug 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 76 additions & 140 deletions modmesh/app/euler1d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,16 @@ def update(self, adata, ndata):
self.num.figure.canvas.draw()


class SolverConfig():
class GUIConfig:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Considering that SolverConfig and PlotConfig have a lot of similar code, I created a parent class to implement the shared parts.

Copy link
Member

Choose a reason for hiding this comment

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

I would hope we keep a minimal Python 2/3 compatibility for classes (although I don't think anyone would use Python 2). That is to make a class derived from object:

class GUIConfig(object):

In Python 3 it is the same as writing class GUIConfig:. But in Python 2, class GUIConfig: is the old-style (pre-2.2) class, and incompatible to the new-style class (2.3 and beyond).

Having the minimal compatibility makes it slight easier for people to copy code to Python 2.

"""
Configuration class for the solver.
Configuration class for the GUI.

This class provides a configuration interface for the solver, allowing
This class provides a configuration interface for the GUI, allowing
users to set and retrieve parameters related to the simulation.

Attributes:
Copy link
Member

Choose a reason for hiding this comment

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

Please use Sphinx format for the attributes in the docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have fixed it in d25d5c2.

- `state` (:class:`State`): The state object holding configuration
data in the form of [variable_name, value, description].
data.
- `_tbl_content` (:class:`State`): The content of the
configuration table.
- `_col_header` (list): The header for the configuration
Expand All @@ -132,13 +132,40 @@ class SolverConfig():
table.
- :meth:`columnCount()`: Get the number of columns in the
configuration table.
- :meth:`get_var(key_row, key_col)`: Get the value of a configuration
variable based on its key.
"""
def __init__(self, input_data):
class Accessor:
Copy link
Member

Choose a reason for hiding this comment

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

Why do you use an inner class? It's less testable. What about moving it outside in the module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My original idea is to achieve __getitem__ chaining by returning an Accessor instance and Accessor is specialize for accessing GUIConfig data. That's why I put this class inside GUIConfig.

If we move Accessor outside, it would imply that Accessor needs to become a more generic class.
I will try to re-design Accessor class.

Copy link
Member

Choose a reason for hiding this comment

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

Being specific or general has nothing to do with making it an inner class or a normal (in-module) class. When making a help class for a specific use inside another class, a common style is to mark it as a private class like:

class _Accessor:
    ....
class Consumer:
    def func(self):
        a = _Accessor(...)
        a.helper_function()

"""
Helper calss to access data within the configuration table using
multiple dimensions, currenlty only support 2-dimensions table.

This class allows users to access data by chaining multiple [],
supporting string-based.
"""
def __init__(self, data, dimIdx=0, header=None):
self._data = data
self._header = header
self._dimIdx = dimIdx

def __getitem__(self, key):
if self._dimIdx == 0:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used to identify whether the [] operator is currently handling row accessing, because row access requires checking the row name in the first column.

for row_idx, row in enumerate(self._data):
if key == row[0]:
return GUIConfig.Accessor(self._data[row_idx],
self._dimIdx+1,
self._header)
else:
for idx, ele in enumerate(self._header):
if key == ele:
return self._data[idx]
raise KeyError(f'Invaild key: {key} not found in table')

def __init__(self, input_data, col_headers):
self.state = State(input_data)
self._tbl_content = self.state
self._col_header = ["variable", "value", "description"]
self._col_header = col_headers

def __getitem__(self, key):
return self.Accessor(self._tbl_content, 0, self._col_header)[key]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems that the logic is simple enough to be a function. I am not sure if using a function can reduce the overhead of creating a class?

Copy link
Member

Choose a reason for hiding this comment

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

I have the same question. But I care more about the maintainability. Both Python functions and classes are slow. And I don't think we plan to profile for it any time soon.


def data(self, row, col):
"""
Expand Down Expand Up @@ -209,25 +236,27 @@ def columnCount(self):
"""
return len(self._tbl_content[0])

def get_var(self, key_row, key_col):
"""
Get the value of a configuration variable based on its key.

:param key_row: The row key of the variable.
:type key_row: str
:param key_col: The col key of the variable.
:type key_col: str
:return: The value of the specified variable.
"""
for ele in self.state:
if key_row == ele[0]:
for idx, name in enumerate(self._col_header):
if key_col == name:
return ele[idx]
return None
class SolverConfig(GUIConfig):
"""
Configuration class for the solver.

This class provides a configuration interface for the solver, allowing
users to set and retrieve parameters related to the simulation.

class PlotConfig():
Attributes:
- `state` (:class:`State`): The state object holding configuration
data in the form of [variable_name, value, description].
- `_tbl_content` (:class:`State`): The content of the
configuration table.
- `_col_header` (list): The header for the configuration
table columns.
"""
def __init__(self, input_data):
super().__init__(input_data, ["variable", "value", "description"])


class PlotConfig(GUIConfig):
"""
Configuration class for the plot.

Expand All @@ -242,66 +271,12 @@ class PlotConfig():
configuration table.
- `_col_header` (list): The header for the configuration
table columns.

Methods:
- :meth:`data(row, col)`: Get the value at a specific row and column
in the configuration table.
- :meth:`setData(row, col, value)`: Set the value at a specific row
and column in the configuration table.
- :meth:`columnHeader(col)`: Get the header for a specific column
in the configuration table.
- :meth:`editable(row, col)`: Check if a cell in the configuration
table is editable.
- :meth:`rowCount()`: Get the number of rows in the configuration
table.
- :meth:`columnCount()`: Get the number of columns in the
configuration table.
- :meth:`get_var(key, key_row, key_col)`: Get the value of a
configuration variable based on its key.
"""
def __init__(self, input_data):
self.state = State(input_data)
self._tbl_content = self.state
self._col_header = ["variable",
"line_selection",
"y_axis_upper_limit",
"y_axis_bottom_limit"]

def data(self, row, col):
"""
Get the value at a specific row and column in the configuration table.

:param row: Row index.
:type row: int
:param col: Column index.
:type col: int
:return: The value at the specified location in
the configuration table.
"""
return self._tbl_content[row][col]

def setData(self, row, col, value):
"""
Set the value at a specific row and column in the configuration table.

:param row: Row index.
:type row: int
:param col: Column index.
:type col: int
:prarm value: Any
:return None
"""
self._tbl_content[row][col] = value

def columnHeader(self, col):
"""
Get the specific column header in the configuration table.

:param col: Column index.
:type col: int
:return: The header for the specific column.
"""
return self._col_header[col]
super().__init__(input_data, ["variable",
"line_selection",
"y_axis_upper_limit",
"y_axis_bottom_limit"])

def editable(self, row, col):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since PlotConfig table has multiple columns allowing user to modified its value via QtTableView. Therefore I overwrite the editable method.

"""
Expand All @@ -317,42 +292,6 @@ def editable(self, row, col):
return True
return False

# Delete row header
rowHeader = None

def rowCount(self):
"""
Get the number of rows in the configuration table.

:return: The number of rows.
"""
return len(self._tbl_content)

def columnCount(self):
"""
Get the number of columns in the configuration table.

:return: The number of columns.
"""
return len(self._tbl_content[0])

def get_var(self, key_row, key_col):
"""
Get the value of a configuration variable based on its key.

:param key_row: The row key of the variable.
:type key_row: str
:param key_col: The col key of the variable.
:type key_col: str
:return: The value of the specified variable.
"""
for ele in self.state:
if key_row == ele[0]:
for idx, name in enumerate(self._col_header):
if key_col == name:
return ele[idx]
return None


class Euler1DApp():
"""
Expand Down Expand Up @@ -493,24 +432,21 @@ def set_solver_config(self):

:return None
"""
self.init_solver(gamma=self.solver_config.get_var("gamma", "value"),
pressure_left=self.solver_config.get_var("p_left",
"value"),
density_left=self.solver_config.get_var("rho_left",
"value"),
pressure_right=self.solver_config.get_var("p_right",
"value"),
density_right=self.solver_config.get_var("rho_right",
"value"),
xmin=self.solver_config.get_var("xmin", "value"),
xmax=self.solver_config.get_var("xmax", "value"),
ncoord=self.solver_config.get_var("ncoord", "value"),
time_increment=(self.solver_config.
get_var("time_increment", "value")))
self.init_solver(gamma=self.solver_config["gamma"]["value"],
pressure_left=self.solver_config["p_left"]["value"],
density_left=self.solver_config["rho_left"]["value"],
pressure_right=self.solver_config["p_right"]["value"],
density_right=self.solver_config["rho_right"]
["value"],
xmin=self.solver_config["xmin"]["value"],
xmax=self.solver_config["xmax"]["value"],
ncoord=self.solver_config["ncoord"]["value"],
time_increment=(self.solver_config["time_increment"][
"value"]))
self.current_step = 0
self.interval = self.solver_config.get_var("timer_interval", "value")
self.max_steps = self.solver_config.get_var("max_steps", "value")
self.profiling = self.solver_config.get_var("profiling", "value")
self.interval = self.solver_config["timer_interval"]["value"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original get_var handles the case if the key doesn't exist.
Perhaps write it as self.solver_config.get("timer_interval", {}). get("value", None)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But the get_var way was already quite wordy, and the new way seems to make it even wordy.

Copy link
Member

Choose a reason for hiding this comment

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

API conciseness is hard, and that is why I developed the toggle code:

modmesh/tests/test_toggle.py

Line 258 in 194a313

self.assertEqual(tg.level1.test_real, 9.42)

It may take some efforts to connect the toggle library with PUI, so I am not promoting it now. But we will need it at some point.

Copy link
Member

Choose a reason for hiding this comment

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

At this point, I'd say __getitem__() may be a a reasonable balance.

self.max_steps = self.solver_config["max_steps"]["value"]
self.profiling = self.solver_config["profiling"]["value"]

def setup_timer(self):
"""
Expand Down Expand Up @@ -592,7 +528,7 @@ def build_single_figure(self):
):
# Plot multiple data line with same X axis, it need to plot a
# data line on main axis first
if self.plot_config.get_var(data.name, "line_selection"):
if self.plot_config[data.name]["line_selection"]:
data.axis = ax
data.ana, = ax.plot(x, np.zeros_like(x),
f'{color}-',
Expand Down Expand Up @@ -683,10 +619,10 @@ def update_layout(self):
self.internal_energy,
self.entropy
):
line.y_upper_lim = self.plot_config.get_var(line.name,
"y_axis_upper_limit")
line.y_bottom_lim = self.plot_config.get_var(line.name,
"y_axis_bottom_limit")
line.y_upper_lim = self.plot_config[line.name][
"y_axis_upper_limit"]
line.y_bottom_lim = self.plot_config[line.name][
"y_axis_bottom_limit"]

if self.use_grid_layout:
self.plot_holder.plot = self.build_grid_figure()
Expand Down
Loading