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.

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

138 virtual boundary method for scalar fields #140

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

armantekinalp
Copy link
Collaborator

@armantekinalp armantekinalp commented Jun 16, 2023

This pull request expands the functionality of the virtual boundary method implemented in Sopht. It introduces support for scalar fields, enabling the simulation and manipulation of variables like temperature within the framework.

Solves #138

New features are

  • Scalar advection and diffusion simulator.
  • Forcing grid for constant temperature rods and rigid bodies.
  • Forcing grid for Neumann type temperature boundary conditions on rods and rigid bodies.
  • Examples to demonstrate and validate above forcing grids.

We changed set fixed field kernels such that for both vector and scalar
field function structures will be same.
@armantekinalp armantekinalp added the enhancement New feature or request label Jun 16, 2023
@armantekinalp armantekinalp self-assigned this Jun 16, 2023
@armantekinalp armantekinalp marked this pull request as draft June 21, 2023 21:40
@armantekinalp armantekinalp removed the request for review from bhosale2 June 21, 2023 21:40
armantekinalp and others added 19 commits June 21, 2023 16:40
@armantekinalp armantekinalp marked this pull request as ready for review September 11, 2023 14:12
Copy link
Collaborator

@sy-cui sy-cui left a comment

Choose a reason for hiding this comment

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

A few minor comments and questions. Make sure the CI is passed.

Comment on lines +87 to +90
def scalar_field_set_fixed_val_pyst_kernel_2d(field, fixed_val):
set_fixed_val_pyst_kernel_2d(field=field, fixed_val=fixed_val)

return scalar_field_set_fixed_val_pyst_kernel_2d
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this functionally different from before?

Comment on lines +87 to +90
def scalar_field_set_fixed_val_pyst_kernel_3d(field, fixed_val):
set_fixed_val_pyst_kernel_3d(field=field, fixed_val=fixed_val)

return scalar_field_set_fixed_val_pyst_kernel_3d
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as 2D comment.

Comment on lines +25 to +31
@ps.kernel
def _update_passive_field_from_forcing_stencil_2d():
passive_field, forcing_field = ps.fields(
f"passive_field, forcing_field " f": {pyst_dtype}[{grid_info}]"
)
prefactor = sp.symbols("prefactor")
passive_field[0, 0] @= passive_field[0, 0] + forcing_field[0, 0] * prefactor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use saxpy for this? It has an extra scalar multiplication but saves this file. Same for 3D.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have a new class instead of using the passive transport class?

data_timer_limit = 0.25 * timescale
drag_coeffs_time = []
drag_coeffs = []
nusslet_number_time = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be 'nusselt' instead of 'nusslet' if you are referring to the Nusselt number. There seems to be 57 occurrences across the files.

import sopht.utils as spu
import sys

sys.path.append("/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this? This seems a bit dangerous. Same for the other files that have this

Comment on lines +616 to +617
self.compute_lag_grid_position_field()
self.compute_lag_grid_velocity_field()
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should probably be abstracted somehow in a later fix considering we are doing this in every initialization even when not necessary. But okay for now.

@armantekinalp armantekinalp marked this pull request as draft June 1, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants