GUI configuration supporting string based access #414

j8xixo12 · 2024-08-19T00:08:53Z

I found that the content of the table in SolverConfig and PlotConfig is only used internally within the class, and QtTableView interacts with the data solely through data and setData methods. Users will only access the data in a string-based manner. In this PR, I have only implemented the string-based accessing part with __getitem__

j8xixo12 · 2024-08-19T00:12:53Z

modmesh/app/euler1d.py

@@ -104,16 +104,16 @@ def update(self, adata, ndata):
        self.num.figure.canvas.draw()


-class SolverConfig():
+class GUIConfig:


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

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.

j8xixo12 · 2024-08-19T00:19:22Z

modmesh/app/euler1d.py

+            self._dimIdx = dimIdx
+
+        def __getitem__(self, key):
+            if self._dimIdx == 0:


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.

j8xixo12 · 2024-08-19T00:21:36Z

modmesh/app/euler1d.py

+        super().__init__(input_data, ["variable",
+                                      "line_selection",
+                                      "y_axis_upper_limit",
+                                      "y_axis_bottom_limit"])

    def editable(self, row, col):


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

tigercosmos · 2024-08-19T00:42:19Z

modmesh/app/euler1d.py

+        self._col_header = col_headers
+
+    def __getitem__(self, key):
+        return self.Accessor(self._tbl_content, 0, self._col_header)[key]


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?

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.

tigercosmos · 2024-08-19T00:46:43Z

modmesh/app/euler1d.py

-        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"]


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)

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

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.

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

yungyuc

It's significantly simpler. I don't mind merging it now. @j8xixo12, please let me know if you want to discuss before merging, or the other way around.

Discuss whether Accessor should be moved outside from inner?

yungyuc · 2024-08-19T14:05:36Z

modmesh/app/euler1d.py

@@ -104,16 +104,16 @@ def update(self, adata, ndata):
        self.num.figure.canvas.draw()


-class SolverConfig():
+class GUIConfig:


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.

yungyuc · 2024-08-19T14:07:49Z

modmesh/app/euler1d.py


-    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:


Please use Sphinx format for the attributes in the docstring.

I have fixed it in d25d5c2.

yungyuc · 2024-08-19T14:08:58Z

modmesh/app/euler1d.py

    """
-    def __init__(self, input_data):
+    class Accessor:


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

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.

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()

yungyuc · 2024-08-19T14:11:46Z

modmesh/app/euler1d.py

+        self._col_header = col_headers
+
+    def __getitem__(self, key):
+        return self.Accessor(self._tbl_content, 0, self._col_header)[key]


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.

yungyuc · 2024-08-21T00:55:07Z

@j8xixo12, please let me know if you want to discuss before merging, or the other way around.

@j8xixo12, could you please let me know which way do you want to go? If you cannot make your mind now, don't hesitate to just say you need some time to think about it.

j8xixo12 · 2024-08-22T10:50:30Z

@j8xixo12, please let me know if you want to discuss before merging, or the other way around.

@j8xixo12, could you please let me know which way do you want to go? If you cannot make your mind now, don't hesitate to just say you need some time to think about it.

I would like to discuss more before merging, and #414 (comment) seems like a good way to proceed.

And I also found that the way of accessing data using indexes and strings seems very similar to a dataframe, and it looks like we could use a dataframe to store table contents in the future.

j8xixo12 · 2024-08-22T12:53:13Z

In latest patch I have completed the following items:

GUIConfig inherit from object for python2 compatible.
Fix docstring style in GUIConfig
Move Accessor out of GUIConfig and mark it as a private class.

yungyuc

And I also found that the way of accessing data using indexes and strings seems very similar to a dataframe, and it looks like we could use a dataframe to store table contents in the future.

If you have hundreds of thousands of parameters to manage, a data frame is a good container. Otherwise, it hurts more than gains.

But we can use more improvement of the GUI code after this PR.

(Since you decide to do it), make every class you added/changed to be Python 2/3 compatible.

yungyuc · 2024-08-23T06:49:11Z

modmesh/app/euler1d.py

@@ -104,41 +104,77 @@ def update(self, adata, ndata):
        self.num.figure.canvas.draw()


-class SolverConfig():
+class _Accessor:


Every class (added/changed in the PR) should follow the Python 2/3 compatible declaration (inherit from object).

j8xixo12 · 2024-08-24T15:57:22Z

And I also found that the way of accessing data using indexes and strings seems very similar to a dataframe, and it looks like we could use a dataframe to store table contents in the future.

If you have hundreds of thousands of parameters to manage, a data frame is a good container. Otherwise, it hurts more than gains.

But we can use more improvement of the GUI code after this PR.

(Since you decide to do it), make every class you added/changed to be Python 2/3 compatible.

I have fixed it.

(Since you decide to do it), make every class you added/changed to be Python 2/3 compatible.

yungyuc · 2024-08-25T02:39:06Z

LGTM. Thanks.

GUI configuration supporting string based access

6d4c3da

j8xixo12 commented Aug 19, 2024

View reviewed changes

tigercosmos reviewed Aug 19, 2024

View reviewed changes

yungyuc approved these changes Aug 19, 2024

View reviewed changes

yungyuc assigned j8xixo12 Aug 19, 2024

yungyuc added viewer Visualize stuff onedim One-dimensional solver labels Aug 19, 2024

move Accessor out of GUIConfig and fix docstring style

d25d5c2

j8xixo12 force-pushed the GUIConfig branch from d2ba5cb to d25d5c2 Compare August 22, 2024 12:47

yungyuc requested changes Aug 23, 2024

View reviewed changes

make class compatible with python 2/3

823342a

yungyuc merged commit 4166c1a into solvcon:master Aug 25, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUI configuration supporting string based access #414

GUI configuration supporting string based access #414

j8xixo12 commented Aug 19, 2024

j8xixo12 Aug 19, 2024

yungyuc Aug 19, 2024

j8xixo12 Aug 19, 2024

j8xixo12 Aug 19, 2024

tigercosmos Aug 19, 2024

yungyuc Aug 19, 2024

tigercosmos Aug 19, 2024

j8xixo12 Aug 20, 2024

yungyuc Aug 20, 2024

yungyuc Aug 20, 2024

yungyuc left a comment •

edited

Loading

yungyuc Aug 19, 2024

yungyuc Aug 19, 2024

j8xixo12 Aug 22, 2024

yungyuc Aug 19, 2024

j8xixo12 Aug 20, 2024

yungyuc Aug 21, 2024

yungyuc Aug 19, 2024

yungyuc commented Aug 21, 2024 •

edited

Loading

j8xixo12 commented Aug 22, 2024 •

edited

Loading

j8xixo12 commented Aug 22, 2024

yungyuc left a comment •

edited

Loading

yungyuc Aug 23, 2024

j8xixo12 Aug 24, 2024

j8xixo12 commented Aug 24, 2024

yungyuc commented Aug 25, 2024

GUI configuration supporting string based access #414

GUI configuration supporting string based access #414

Conversation

j8xixo12 commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungyuc commented Aug 21, 2024 • edited Loading

j8xixo12 commented Aug 22, 2024 • edited Loading

j8xixo12 commented Aug 22, 2024

yungyuc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j8xixo12 commented Aug 24, 2024

yungyuc commented Aug 25, 2024

yungyuc left a comment •

edited

Loading

yungyuc commented Aug 21, 2024 •

edited

Loading

j8xixo12 commented Aug 22, 2024 •

edited

Loading

yungyuc left a comment •

edited

Loading