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

Init bugs2 #1142

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from
Draft

Init bugs2 #1142

wants to merge 13 commits into from

Conversation

aronroland
Copy link
Collaborator

@aronroland aronroland commented Dec 16, 2023

Pull Request Summary

Solves uninitialized variables issues and improves/fixes issues with the Neuman boundary condition.

Description

This pull request, solves uninitialized variable issues for TR0 and other bugs connected to initialization issues. Moreover, adding Neumann-type boundary conditions in the RD framework can be accomplished by calculating the solution at the boundary, without modifying the scheme itself. Specifically, a zero-gradient Neumann condition is applied at the boundary. However, this approach becomes less straightforward when dealing with positive source term contributions or phenomena like refraction and frequency shifting. In such scenarios, the Neumann boundary condition, characterized by a zero gradient, is not sufficiently defined. This is because the waves at the boundary may grow, change direction, or modulate, leading to a net influx of energy into the domain. To resolve this, we have developed a solution that involves bypassing the calculation of spectral propagation at Neumann boundary points and excluding the computation of positive source terms.

Please also include the following information:
The code changes are trivial.
It was a bug.
Answers may change for the regtest, where the certain settings are used.

Issue(s) addressed

none issue was raised yet.

Commit Message

Fixing uninitialized issues within the implicit scheme

Check list

Testing

  • How were these changes tested?
    I have checked on certain setups by SHOM (with Heloise Michaud), that this fixes the issues she has found.
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?
    Will be done by ERDC.
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
    Mostly none ...
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):
    Will be done by ERDC

@JessicaMeixner-NOAA
Copy link
Collaborator

@aronroland thank you for submitting this PR! As soon as the testing information from ERDC is added we'll start the tests and review.

@JessicaMeixner-NOAA
Copy link
Collaborator

Converting this PR to a draft. Please mark as ready to review with testing information is available. @aronroland @thesser1

@JessicaMeixner-NOAA JessicaMeixner-NOAA marked this pull request as draft January 16, 2024 15:36
@JessicaMeixner-NOAA
Copy link
Collaborator

@aronroland @thesser1 will you help make sure that the description of this PR includes all the updates in this branch or confirm that it's all there already?

@aronroland
Copy link
Collaborator Author

sure ...

@thesser1
Copy link
Collaborator

Please see attached results from regtests. Looks like binary differences.
matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt

@MatthewMasarik-NOAA
Copy link
Collaborator

Please see attached results from regtests. Looks like binary differences. matrixCompFull.txt matrixCompSummary.txt matrixDiff.txt

Thanks for adding the testing @thesser1, I'll start working on it.

@JessicaMeixner-NOAA
Copy link
Collaborator

@thesser1 is this ready to be marked ready for review?

I did see that some tests had differences in the output of the unstructured grid cases, is this expected with the bug fixes?

THR = DBLE(1.E-15)
IF (SUM(A) .LT. THR) RETURN
IF (EMEAN .LT. TINY(1.d0)) THEN
S = 0
Copy link
Collaborator

@ukmo-ccbunney ukmo-ccbunney Mar 27, 2024

Choose a reason for hiding this comment

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

S and D are zeroed above. Why zero again here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this was some leftover i cleaned it now, thanks @ukmo-ccbunney

@MatthewMasarik-NOAA
Copy link
Collaborator

@thesser1, I'm going to pause looking at this while there are some questions from Jessica and Chris.

@thesser1
Copy link
Collaborator

All, I had an issue with my netcdf library when I ran matrix.comp. I just noticed it and reran the comparison, and it increased the number of differences. Sorry for posting before checking. Based on comments above, it looks like there will be a fix and rerun needed, but I wanted to make sure the files posted here reflected all regression issues. I am sending differences to Aron, but I would keep this as draft for the time being.

@MatthewMasarik-NOAA
Copy link
Collaborator

All, I had an issue with my netcdf library when I ran matrix.comp. I just noticed it and reran the comparison, and it increased the number of differences. Sorry for posting before checking. Based on comments above, it looks like there will be a fix and rerun needed, but I wanted to make sure the files posted here reflected all regression issues. I am sending differences to Aron, but I would keep this as draft for the time being.

Thanks for the update, @thesser1. Please keep us posted

@aronroland
Copy link
Collaborator Author

Hi @JessicaMeixner-NOAA, we experience all HPCF maintenance. I guess it will be next week. Have a good weekend!

@JessicaMeixner-NOAA
Copy link
Collaborator

@aronroland thanks for the update!

@JessicaMeixner-NOAA
Copy link
Collaborator

@aronroland - I just wanted to check in to see if there were any updates on this PR.

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.

5 participants