Skip to content

Hide boundary points in figure #391

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

Merged
yungyuc merged 6 commits into from
Oct 5, 2024
Merged

Hide boundary points in figure #391

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f30cb6d
hide boundary points in figure
Jul 21, 2024
059272a
set xindices as an option
Jul 25, 2024
bae5f8e
keep boundary points in test_field_with_numerical
Aug 7, 2024
ff4fac0
format document with black
Aug 8, 2024
e2de8d0
init xindices in Euler1DSolver
Sep 25, 2024
0936eb9
refactor xindices
Oct 5, 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
43 changes: 27 additions & 16 deletions modmesh/app/euler1d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ def build_grid_figure(self):

:return: FigureCanvas
"""
x = self.st.svr.coord[::2]
x = self.st.svr.coord[self.st.svr.xindices]
fig = Figure()
canvas = FigureCanvas(fig)
ax = canvas.figure.subplots(3, 2)
Expand Down Expand Up @@ -574,7 +574,7 @@ def build_single_figure(self):

:return: FigureCanvas
"""
x = self.st.svr.coord[::2]
x = self.st.svr.coord[self.st.svr.xindices]
fig = Figure()
canvas = FigureCanvas(fig)
ax = canvas.figure.subplots()
Expand Down Expand Up @@ -743,24 +743,35 @@ def update_lines(self):
:return: None
"""
if self.use_grid_layout:
self.density.update(adata=self.st.density_field,
ndata=self.st.svr.density[::2])
self.pressure.update(adata=self.st.pressure_field,
ndata=self.st.svr.pressure[::2])
self.velocity.update(adata=self.st.velocity_field,
ndata=self.st.svr.velocity[::2])
self.temperature.update(adata=self.st.temperature_field,
ndata=self.st.svr.temperature[::2])
self.internal_energy.update(adata=(self.st.internal_energy_field),
ndata=(self.st.svr.
internal_energy[::2]))
self.entropy.update(adata=self.st.entropy_field,
ndata=self.st.svr.entropy[::2])
self.density.update(
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@yungyuc yungyuc Sep 29, 2024

Choose a reason for hiding this comment

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

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.

adata=self.st.density_field,
ndata=self.st.svr.density[self.st.svr.xindices]
)
self.pressure.update(
adata=self.st.pressure_field,
ndata=self.st.svr.pressure[self.st.svr.xindices]
)
self.velocity.update(
adata=self.st.velocity_field,
ndata=self.st.svr.velocity[self.st.svr.xindices]
)
self.temperature.update(
adata=self.st.temperature_field,
ndata=self.st.svr.temperature[self.st.svr.xindices]
)
self.internal_energy.update(
adata=(self.st.internal_energy_field),
ndata=(self.st.svr.internal_energy[self.st.svr.xindices])
)
self.entropy.update(
adata=self.st.entropy_field,
ndata=self.st.svr.entropy[self.st.svr.xindices]
)
else:
for name, is_selected, *_ in self.plot_config.state:
if is_selected:
eval(f'(self.{name}.update(adata=self.st.{name}_field,'
f' ndata=self.st.svr.{name}[::2]))')
f' ndata=self.st.svr.{name}[self.st.svr.xindices]))')


class PlotArea(PuiInQt):
Expand Down
15 changes: 13 additions & 2 deletions modmesh/onedim/euler1d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,16 +302,27 @@ def calc_internal_energy(self, pressure, density):
def calc_entropy(self, pressure, density):
return pressure / (density ** self.gamma)

def build_field(self, t, coord=None):
def build_field(self, t, coord=None, keep_edge=False):
"""
Populate the field data using the analytical solution.

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

Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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

if None is coord:
coord = self.svr.coord[::2] # Use the numerical solver.
_ = self.svr.ncoord
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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

if keep_edge:
self.svr.xindices = np.linspace(
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

@Gene0315 Gene0315 Sep 15, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@yungyuc yungyuc Sep 16, 2024

Choose a reason for hiding this comment

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

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.

0, (_ - 1), num=((_ + 1) // 2), dtype=int
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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

)
else:
self.svr.xindices = np.linspace(
2, (_ - 3), num=((_ - 3) // 2), dtype=int
)
# Use the numerical solver.
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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

coord = self.svr.coord[self.svr.xindices]
self.coord = coord.copy() # Make a copy; no write back to argument.

# Determine the zone location and the Boolean selection arrays.
Expand Down
3 changes: 3 additions & 0 deletions startup.py
View file
Open in desktop
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

This is redundant file, please remove.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import modmesh.view as mv

mv.launch()
2 changes: 1 addition & 1 deletion tests/test_onedim_euler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def test_field_without_numerical(self):
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.st.build_field(t=0.0, keep_edge=True)
Copy link
Copy Markdown
Member

@yungyuc yungyuc Aug 10, 2024

Choose a reason for hiding this comment

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

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

self.assertIsInstance(self.st.svr, euler1d.Euler1DSolver)
self.assertEqual(len(self.st.coord), 11)
self._check_field_value_at_t0()
Expand Down