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 bugs #1073

Closed
wants to merge 2 commits into from
Closed

Init bugs #1073

wants to merge 2 commits into from

Conversation

aronroland
Copy link
Collaborator

@aronroland aronroland commented Sep 12, 2023

Pull Request Summary

This solves issue #1071

Description

See #1071

Please also include the following information:
Dear reviewer, as described in #1071 bugs have been found for the case when certain source terms or all source terms are switched off within the implicit UG scheme. The bugs have been solved and tested within the cases provided by ERDC.

  • Mention any labels that should be added:
    bug

  • Are answer changes expected from this PR? Please describe the changes and the reason why in addition to which of the following labels would apply:

Reg-tests may change where the described settings are used.

Issue(s) addressed

See #1071

Commit Message

Non initialized Arrays for TR0, DB0 or FLSOU = .false. and erroneous boundary handling of the source terms for the Neumann boundary condtion on unstructured grids (implicit scheme)

Check list

Testing

  • How were these changes tested?
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)?
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

Regression tests, will be done within the PR review ...

@MatthewMasarik-NOAA
Copy link
Collaborator

MatthewMasarik-NOAA commented Sep 12, 2023

Hi @aronroland, thanks for submitting this bugfix PR!

Its sounds like you did testing with ERDC for this. Is there a reason though that the regtests haven't been run? This should be done so you can fill out the PR header with specifically what tests have changed answers so we already have that information when we do testing for the review.

@aronroland
Copy link
Collaborator Author

Dear Mathew, as I have no computational resources to carry out the regtest we will do that within the review, please give us some time to do so.

@MatthewMasarik-NOAA
Copy link
Collaborator

Hi @aronroland, sure, can you clarify if you are asking for time to allow you guys to run the tests?

@aronroland
Copy link
Collaborator Author

yes, exactly this what i am asking for.

@MatthewMasarik-NOAA
Copy link
Collaborator

Sure thing, then.  There are a few PR's in front of yours, so that gives some time already before your's is up.  If we get to your's and you would still like more time, no problem, we can review it once you're ready.  Thank you @aronroland

@aronroland
Copy link
Collaborator Author

thank u, Matthew ...

@ukmo-ccbunney
Copy link
Collaborator

I'm watching this PR with interest as I am currently experiencing B4B reproducibility issues with some of the PDLIB regression tests after my refactoring of W3SRCE. I am hoping this PR might make them disappear! 🤞

@aronroland
Copy link
Collaborator Author

aronroland commented Sep 13, 2023 via email

Copy link
Collaborator

@MatthewMasarik-NOAA MatthewMasarik-NOAA left a comment

Choose a reason for hiding this comment

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

@aronroland We can start reviewing once the testing results have been added. Thanks

@MatthewMasarik-NOAA
Copy link
Collaborator

@aronroland please sync this branch for merged pr #1070.

@MatthewMasarik-NOAA MatthewMasarik-NOAA mentioned this pull request Sep 21, 2023
4 tasks
@MatthewMasarik-NOAA
Copy link
Collaborator

@thesser1 @aronroland I know you guys are testing, note that this branch needs to be synced to be current.

@MatthewMasarik-NOAA
Copy link
Collaborator

Closing on behalf of @thesser1 and @aronroland.

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.

3 participants