-
Notifications
You must be signed in to change notification settings - Fork 41
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
GUI configuration supporting string based access #414
Conversation
modmesh/app/euler1d.py
Outdated
@@ -104,16 +104,16 @@ def update(self, adata, ndata): | |||
self.num.figure.canvas.draw() | |||
|
|||
|
|||
class SolverConfig(): | |||
class GUIConfig: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
modmesh/app/euler1d.py
Outdated
self._dimIdx = dimIdx | ||
|
||
def __getitem__(self, key): | ||
if self._dimIdx == 0: |
There was a problem hiding this comment.
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.
super().__init__(input_data, ["variable", | ||
"line_selection", | ||
"y_axis_upper_limit", | ||
"y_axis_bottom_limit"]) | ||
|
||
def editable(self, row, col): |
There was a problem hiding this comment.
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.
modmesh/app/euler1d.py
Outdated
self._col_header = col_headers | ||
|
||
def __getitem__(self, key): | ||
return self.Accessor(self._tbl_content, 0, self._col_header)[key] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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"] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
modmesh/app/euler1d.py
Outdated
@@ -104,16 +104,16 @@ def update(self, adata, ndata): | |||
self.num.figure.canvas.draw() | |||
|
|||
|
|||
class SolverConfig(): | |||
class GUIConfig: |
There was a problem hiding this comment.
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.
|
||
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
modmesh/app/euler1d.py
Outdated
""" | ||
def __init__(self, input_data): | ||
class Accessor: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
modmesh/app/euler1d.py
Outdated
self._col_header = col_headers | ||
|
||
def __getitem__(self, key): | ||
return self.Accessor(self._tbl_content, 0, self._col_header)[key] |
There was a problem hiding this comment.
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.
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. |
In latest patch I have completed the following items:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
modmesh/app/euler1d.py
Outdated
@@ -104,41 +104,77 @@ def update(self, adata, ndata): | |||
self.num.figure.canvas.draw() | |||
|
|||
|
|||
class SolverConfig(): | |||
class _Accessor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every class (added/changed in the PR) should follow the Python 2/3 compatible declaration (inherit from object
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
I have fixed it.
|
LGTM. Thanks. |
I found that the content of the table in
SolverConfig
andPlotConfig
is only used internally within the class, andQtTableView
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__