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

Use inheritance for check_vertical_flux #104

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Use inheritance for check_vertical_flux #104

merged 1 commit into from
Nov 25, 2024

Conversation

daubners
Copy link
Contributor

@daubners daubners commented Nov 7, 2024

  • Removed some artefacts from previous merge
  • adjusted calc_vertical_flux for each solver such that
  • check_vertical_flux can be inherited from base solver

@amirDahari1 amirDahari1 self-requested a review November 15, 2024 18:58
def check_vertical_flux(self, conv_crit):
vert_flux = self.calc_vertical_flux()
fl = torch.sum(vert_flux, (0, 2, 3))[1:-1]
fl = torch.sum(vert_flux, (0, 2, 3))
Copy link
Member

Choose a reason for hiding this comment

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

Explain why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the check_vertical_flux functions only differed in the way the field was cropped (see deleted code below) but all the other code lines where duplicated. If the calc_vertical_flux already gives back the right size, there should be no need for cropping and, thus the function can be inherited to all other solvers.

self.conc[:, :-2, 1:-1, 1:-1]
vert_flux[self.conc[:, :-2, 1:-1, 1:-1] == 0] = 0
vert_flux[self.conc[:, 1:-1, 1:-1, 1:-1] == 0] = 0
# Indexing removes boundary layers (1 layer at every boundary)
Copy link
Member

Choose a reason for hiding this comment

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

Please write another sentence here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input image has the size Nx, Ny, Nz. The field for solving the PDE is Nx+2, Ny+2 and Nz+2 to add cells for boundary conditions. The flux in x-direction should only be computed in the inner of the domain (excluding boundary cells). Furthermore, the flux field is computed from the concentration gradient and, thus, should have dimensions Nx-1, Ny, Nz. This was inconsistent before between all the different solver. Now every solver, no matter how many boundary cells gives back a flux field with size [Nx-1, Ny, Nz]. This is also the reason for the simpler code in the next comment.

vert_flux[self.conc == 0] = 0
vert_flux[torch.roll(self.conc, 1, 1) == 0] = 0
# Indexing removes 2 boundary layers at top and bottom
vert_flux = self.conc[:, 3:-2] - self.conc[:, 2:-3]
Copy link
Member

Choose a reason for hiding this comment

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

aren't you neglecting the .roll element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced the roll with indexing as it was easier to control the size of the vert_flux output field. The flux in x should always be right minus left neighbour, i.e. here 3:-2 minus 2:-3. Before the flux direction was the wrong way and taking the abs value distorts the actual convergence. In the converged state there should only be flux in positive x direction but before, there can be back-flux in dead-end features which should be accounted for with the correct sign and direction

@daubners daubners requested a review from amirDahari1 November 25, 2024 15:28
@amirDahari1 amirDahari1 merged commit 241e554 into main Nov 25, 2024
4 checks passed
@amirDahari1 amirDahari1 deleted the refactor branch November 25, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants