Hide boundary points in figure #391

Gene0315 · 2024-07-21T04:04:35Z

Following the issue #361
The PR hides the boundary points.

Features: The plotting hides the unreasonable points on the boundary.
Strength: It makes the figure look nicer without changing the solution data.
Weakness: It is better to calculate the correct values on the boundary and show them on the boundary.

Gene0315 · 2024-07-21T04:05:55Z

This PR removed the unrelated files in #361 and set the shown points as a variable of Euler1DApp.
However, I still failed to make the plot_point as an universal variable for modmesh/app/euler1d.py and modmesh/onedim/euler1d.py. Is it necessary to do this?

yungyuc

Provide an option to select the amount of data to plot. It can serve as a way to fall back to the original behavior.
Rename plot_point to xindices.
Fix CI errors.

Please fix the CI failures. A prominent one is test failures on the array size: https://github.com/solvcon/modmesh/actions/runs/10025477375/job/27708689299?pr=391#step:11:285 :

=================================== FAILURES ===================================
____________________ ShockTubeTC.test_field_with_numerical _____________________

self = <test_onedim_euler.ShockTubeTC testMethod=test_field_with_numerical>

    def test_field_with_numerical(self):
        self.st.build_numerical(xmin=-1, xmax=1, ncoord=21,
                                time_increment=0.05)
        self.st.build_field(t=0.0)
        self.assertIsInstance(self.st.svr, euler1d.Euler1DSolver)
>       self.assertEqual(len(self.st.coord), 11)
E       AssertionError: 9 != 11

tests/test_onedim_euler.py:110: AssertionError
=========================== short test summary info ============================
FAILED tests/test_onedim_euler.py::ShockTubeTC::test_field_with_numerical - AssertionError: 9 != 11
=================== 1 failed, 181 passed, 5 skipped in 6.72s ===================
make: *** [pytest] Error 1

The test fails because you changed the length of the array.

yungyuc · 2024-07-21T06:52:10Z

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")
+        ncoord = self.solver_config.get_var("ncoord", "value")


@j8xixo12 With this PR I realized that self.solver_config.get_var() is too wordy. The lengthy API makes it hard to quickly write and understand code.

Please propose a new design of SolverConfig and PlotConfig in the discussions. We can probably consider __getattr__() or __getitem__()

@Gene0315 please provide an option to keep existing behavior.

@Gene0315 You can use the inline comment for focused discussions.

@j8xixo12 Any comments?

Ok, I will create a discussion and propose a new design in the discussion.

Ok, I will create a discussion and propose a new design in the discussion.

Thanks, @j8xixo12 . Maybe creating an issue describing what we know and may be done can help.

yungyuc · 2024-07-21T06:55:42Z

modmesh/app/euler1d.py

        self.max_steps = self.solver_config.get_var("max_steps", "value")
        self.profiling = self.solver_config.get_var("profiling", "value")
+        ncoord = self.solver_config.get_var("ncoord", "value")
+        self.plot_point = np.linspace(2, (ncoord - 3), num=((ncoord - 3) // 2), dtype=int)


A convention of short-living temporary variables is _. The two lines can be changed to:

_ = self.solver_config.get_var("ncoord", "value") self.xindices = np.linspace(2, (_ - 3), num=((_ - 3) // 2), dtype=int)

plot_point is not a good attribute name because the content is not points but indices of the coordinate (1D point) arrays.

Gene0315 · 2024-07-22T01:53:25Z

@yungyuc

Provide an option to select the amount of data to plot. It can serve as a way to fall back to the original behavior.

@Gene0315 please provide an option to keep existing behavior.

Are these two the same questions? Do you mean to provide it as an option such as "ncoord" or "max step" in qt window?

Rename plot_point to xindices.

It is ok.

Fix CI errors.

But I don't know how to check whether I solve the CI error correctly. Maybe I can create a PR in my own repo again before I upload the new code here?

yungyuc · 2024-07-22T01:56:41Z

@yungyuc

Provide an option to select the amount of data to plot. It can serve as a way to fall back to the original behavior.

@Gene0315 please provide an option to keep existing behavior.

Are these two the same questions? Do you mean to provide it as an option such as "ncoord" or "max step" in qt window?

Yes they are similar. No, the options only need to be in the code. We do not need them to be in the GUI yet.

Rename plot_point to xindices.

It is ok.

Fix CI errors.

But I don't know how to check whether I solve the CI error correctly. Maybe I can create a PR in my own repo again before I upload the new code here?

You can turn on Github Actions in your own repository and you will see the CI runs when you push to your repository. You do not need a PR for the CI runs in your repository. You should also run the tests locally to reproduce the failures on your development machine for fixing them.

Gene0315 · 2024-07-22T15:40:13Z

Yes they are similar. No, the options only need to be in the code. We do not need them to be in the GUI yet.

I'm so confused how to set the xindices as an option in the code. Can you give me an example? Just like which variable is an option in the code.

You can turn on Github Actions in your own repository and you will see the CI runs when you push to your repository. You do not need a PR for the CI runs in your repository. You should also run the tests locally to reproduce the failures on your development machine for fixing them.

How can I run the check tests in my laptop? Is it feasable to run the test_onedim_euler1d.py or other test python files directly?

yungyuc · 2024-07-23T02:11:50Z

Yes they are similar. No, the options only need to be in the code. We do not need them to be in the GUI yet.

I'm so confused how to set the xindices as an option in the code. Can you give me an example? Just like which variable is an option in the code.

Something like:

def set_solver_config(self, keep_edge=False):
    # ...
    if keep_edge:
        # create self.xindices including the edge points.
    else:
        _ = self.solver_config.get_var("ncoord", "value")
        self.xindices = np.linspace(2, (_ - 3), num=((_ - 3) // 2), dtype=int)

In this way you can allow a configuring function to turn on keep_edge. The consumer of the solver can decide whether to plot the edge points through that argument.

If you want to make the option available in GUI, the flag keep_edge can be moved where the PUI control can recognize. No matter what you do, you should make unit tests pass.

You can turn on Github Actions in your own repository and you will see the CI runs when you push to your repository. You do not need a PR for the CI runs in your repository. You should also run the tests locally to reproduce the failures on your development machine for fixing them.

How can I run the check tests in my laptop? Is it feasable to run the test_onedim_euler1d.py or other test python files directly?

The Makefile has many targets exercising the tests:

modmesh/Makefile

Line 116 in 6a9888d

.PHONY: pytest

They are used in Github Actions too.

Gene0315 · 2024-08-10T07:59:31Z

@yungyuc I passed all the checks finally! But, I don't know what I have to do next. Could you give me some tips?

yungyuc

@yungyuc I passed all the checks finally! But, I don't know what I have to do next. Could you give me some tips?

Congratulations! Passing all checks means your code starts to work. Then we can start code review.

Please address the following points:

Remove the redundant startup.py.
Use a more specific code comment than "Use the numerical solver".
Remove all unnecessary add/removal/change of blank lines and white spaces.
It makes more sense to calculate num from start and stop.
There should be only one call to np.linspace(). The conditional (if/else) should be used to calculate the arguments to the helper.
The xindices attribute should be created in the initialization helper Euler1DSolver.init_solver() of the object self.svr.
Reduce the change to the code in the function update_lines().

@j8xixo12 Could you please also help take a look?

yungyuc · 2024-08-10T08:51:42Z

startup.py

This is redundant file, please remove.

yungyuc · 2024-08-10T08:54:52Z

modmesh/onedim/euler1d.py

+                self.svr.xindices = np.linspace(
+                    2, (_ - 3), num=((_ - 3) // 2), dtype=int
+                )
+            # Use the numerical solver.


Please be more specific about what you meant. "Use the numerical solver" is not a clear statement.

yungyuc · 2024-08-10T08:56:03Z

modmesh/onedim/euler1d.py

        :param coord: If None, take the coordinate from the numerical solver.
        :return: None
        """
+


This insertion of a blank line is not necessary. Do you introduce unnecessary/unrelated change of blank lines and whitespaces in a patch (i.e., checkins/PR).

yungyuc · 2024-08-10T08:57:03Z

tests/test_onedim_euler.py

        self.st.build_numerical(xmin=-1, xmax=1, ncoord=21,
                                time_increment=0.05)
-        self.st.build_field(t=0.0)
+        self.st.build_field(t=0.0, keep_edge=True)


The fix of the unit tests by the addition of the single argument is good.

yungyuc · 2024-08-10T09:07:49Z

modmesh/onedim/euler1d.py

+
        if None is coord:
-            coord = self.svr.coord[::2]  # Use the numerical solver.
+            _ = self.svr.ncoord


This 9 lines of code is mouthy. Please use the follow code instead (please make proper adjustment if needed because I didn't test it myself):

_ = self.svr.ncoord start = 0 if keep_edge else 2 stop = _ - stop - 1 num = # It makes more sense to calculate from start and stop xindices = np.linspace(start, stop, num=num, dtype='int32')

yungyuc · 2024-08-10T09:08:11Z

modmesh/onedim/euler1d.py

+            _ = self.svr.ncoord
+            if keep_edge:
+                self.svr.xindices = np.linspace(
+                    0, (_ - 1), num=((_ + 1) // 2), dtype=int


It makes more sense to calculate num from start and stop.

yungyuc · 2024-08-10T09:15:41Z

modmesh/onedim/euler1d.py

-            coord = self.svr.coord[::2]  # Use the numerical solver.
+            _ = self.svr.ncoord
+            if keep_edge:
+                self.svr.xindices = np.linspace(


The xindices attribute should be created in the initialization helper Euler1DSolver.init_solver() of the object self.svr:

modmesh/modmesh/onedim/euler1d.py

Line 36 in 214b474

def init_solver(xmin, xmax, ncoord, time_increment, gamma):

Do not add attributes outside the class. It will be very difficult for a maintainer to keep track of where attributes are added, if they are not created in an organized way.

Do not add attributes outside the class. It will be very difficult for a maintainer to keep track of where attributes are added, if they are not created in an organized way.

@yungyuc I agree that we shouldn't add attributes outside the class, but I failed to create the xindices in the Euler1DCore

modmesh/modmesh/onedim/euler1d.py

Line 38 in 214b474

svr = _impl.Euler1DCore(ncoord=ncoord, time_increment=time_increment)

I tried to add the xindices in

modmesh/cpp/modmesh/onedim/Euler1DCore.cpp

Line 45 in 214b474

void Euler1DCore::initialize_data(size_t ncoord)

like m_coord in

modmesh/cpp/modmesh/onedim/Euler1DCore.cpp

Line 52 in 214b474

m_coord = SimpleArray<double>(/*length*/ ncoord);

but I'm confused how to add it because I cannot decide its size in the begining.

Or, maybe I misunderstand what you want me to do? Can you give me some suggestion?

self.svr is a Euler1DSolver object. You can create xindices after

modmesh/modmesh/onedim/euler1d.py

Line 28 in 214b474

self._core = self.init_solver(xmin, xmax, ncoord, time_increment,

The code may be like:

def __init__(self, xmin, xmax, ncoord, time_increment=0.05): self._core = self.init_solver(xmin, xmax, ncoord, time_increment, gamma=1.4) self.xindices = ... # gamma is 1.4 for air.

You may consider to let Euler1DSolver.__init__() take a new argument for keep_edge, if necessary.

yungyuc · 2024-08-10T09:18:56Z

modmesh/app/euler1d.py

-                                               internal_energy[::2]))
-            self.entropy.update(adata=self.st.entropy_field,
-                                ndata=self.st.svr.entropy[::2])
+            self.density.update(


Please simplify the code like:

_s = self.st.svr.xindices self.density.update(adata=self.st.density_field, ndata=self.st.svr.density[_s])

In this way, you do not change the original code by too much. It will be easier to keep track of the change.

Change of line breaking is not necessary for updating the slicing (from [::2] to [_s]). Please revert it to the original style.

That is, since the original style is:

self.pressure.update(adata=self.st.pressure_field, ndata=self.st.svr.pressure[::2])

instead of writing (with the different line breaking):

self.pressure.update( adata=self.st.density_field, ndata=self.st.svr.density[_s] )

write

self.pressure.update(adata=self.st.pressure_field, ndata=self.st.svr.pressure[_s])

The unnecessary change will baffle future maintainers. Style matters. Don't change the style when changing the logic, and vice versa. Change of logic and that of style should not take place at the same time.

Gene0315 · 2024-09-28T08:43:06Z

@yungyuc I tried to revise the code like what you mentioned above. Could you take a look at it? I'm not sure whether the code is fine now. Thank you!

yungyuc

Some old issues were not handled and there are also new issues. Please fix.

app/euler1d.py:747: Do not change the line breaking (style). This has been request in the previous review.
onedim/euler1d.py:324: Do not create a new object for xindices in self.svr when updating the value. Instead, force to use the existing one by writing self.svr.xindices[...].
onedim/euler1d.py:320: Why duplicate code?

yungyuc · 2024-09-29T11:26:18Z

modmesh/app/euler1d.py

-                                               internal_energy[::2]))
-            self.entropy.update(adata=self.st.entropy_field,
-                                ndata=self.st.svr.entropy[::2])
+            self.density.update(


Change of line breaking is not necessary for updating the slicing (from [::2] to [_s]). Please revert it to the original style.

That is, since the original style is:

self.pressure.update(adata=self.st.pressure_field, ndata=self.st.svr.pressure[::2])

instead of writing (with the different line breaking):

self.pressure.update( adata=self.st.density_field, ndata=self.st.svr.density[_s] )

write

self.pressure.update(adata=self.st.pressure_field, ndata=self.st.svr.pressure[_s])

The unnecessary change will baffle future maintainers. Style matters. Don't change the style when changing the logic, and vice versa. Change of logic and that of style should not take place at the same time.

yungyuc · 2024-09-29T11:33:40Z

modmesh/onedim/euler1d.py

+            start = 0 if keep_edge else 2
+            stop = _ if keep_edge else (_ - 2)
+            num = (stop - start) // 2 + 1
+            self.svr.xindices = np.linspace(start, stop, num, dtype='int32')


Write self.svr.xindices[...] = np.linspace(start, stop, num, dtype='int32'). Note that the first ... in [] is ellipsis. By using the ellipsis, you are using the xindices created in the solver object instead of creating a new one and replacing the existing one.

yungyuc · 2024-09-29T13:05:34Z

modmesh/onedim/euler1d.py

-        if None is coord:
-            coord = self.svr.coord[::2]  # Use the numerical solver.
+        if coord is None:
+            _ = self.svr.ncoord - 1


Why do you need to repeat the same code here and in line 32?

Gene0315 · 2024-10-05T07:28:17Z

Some old issues were not handled and there are also new issues. Please fix.

app/euler1d.py:747: Do not change the line breaking (style). This has been request in the previous review.

Sorry, in my understanding, passing make flake8 means the writing style is fine. I will notice it more carefully next time.

onedim/euler1d.py:324: Do not create a new object for xindices in self.svr when updating the value. Instead, force to use the existing one by writing self.svr.xindices[...].

onedim/euler1d.py:320: Why duplicate code?

In previous code, I want to know the size of xindices when init, but I still need to reset it in different conditions with keep_edge. Thus, the code seems to be repeated.
Now, I tried to move the option keep_edge from function build_field to build_numerical and create the xindices in Euler1DSolver directly to avoid the code repeat.

yungyuc

Thanks. It now looks perfect!

hide boundary points in figure

f30cb6d

yungyuc added pilot GUI and visualization onedim One-dimensional solver labels Jul 21, 2024

yungyuc requested changes Jul 21, 2024

View reviewed changes

yungyuc assigned Gene0315 Jul 21, 2024

Gene Lin added 3 commits July 25, 2024 17:29

set xindices as an option

059272a

keep boundary points in test_field_with_numerical

bae5f8e

format document with black

ff4fac0

yungyuc requested changes Aug 10, 2024

View reviewed changes

init xindices in Euler1DSolver

e2de8d0

Gene0315 force-pushed the NRBC_plot branch from bf9976e to e2de8d0 Compare September 25, 2024 13:25

yungyuc requested changes Sep 29, 2024

View reviewed changes

refactor xindices

0936eb9

yungyuc approved these changes Oct 5, 2024

View reviewed changes

yungyuc merged commit 180e911 into solvcon:master Oct 5, 2024

Hide boundary points in figure #391

Hide boundary points in figure #391

Uh oh!

Conversation

Gene0315 commented Jul 21, 2024

Uh oh!

Gene0315 commented Jul 21, 2024

Uh oh!

yungyuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gene0315 commented Jul 22, 2024

Uh oh!

yungyuc commented Jul 22, 2024

Uh oh!

Gene0315 commented Jul 22, 2024

Uh oh!

yungyuc commented Jul 23, 2024

Uh oh!

Gene0315 commented Aug 10, 2024

Uh oh!

yungyuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gene0315 commented Sep 28, 2024

Uh oh!

yungyuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gene0315 commented Oct 5, 2024

Uh oh!

yungyuc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

yungyuc left a comment •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc left a comment •

edited

Loading