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.

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

Jump to bottom

Rheology parameters not set #720

Open
einola opened this issue Oct 22, 2024 · 6 comments
Open

Rheology parameters not set #720

einola opened this issue Oct 22, 2024 · 6 comments
Assignees
@einola @timspainNERSC
Labels
bug Something isn't working

Comments

@einola
Copy link
Member

einola commented Oct 22, 2024

The rheology parameters in param all have the value 0 (or 0.) now.

I check this by putting an assert(params.P0 > 0.) somewhere in BBMStressUpdateStep.hpp (e.g. line 90).

Doing a git bisect reveals that

d5d88649d0f55438fb4f6299c2874ae2a1cb1f28 is the first bad commit
commit d5d88649d0f55438fb4f6299c2874ae2a1cb1f28
Author: Tim Spain <timothy.spain@nersc.no>
Date:   Mon Aug 26 12:04:36 2024 +0200

    Move calculation of the ice ocean stress to a common routine.

 dynamics/src/CGDynamicsKernel.cpp                | 25 ++++++++++++++--
 dynamics/src/include/BrittleCGDynamicsKernel.hpp | 37 +++---------------------
 dynamics/src/include/CGDynamicsKernel.hpp        | 24 +++++++++++++--
 dynamics/src/include/DynamicsKernel.hpp          | 32 ++++++++++++++++----
 dynamics/src/include/FreeDriftDynamicsKernel.hpp | 34 +++-------------------
 dynamics/src/include/VPCGDynamicsKernel.hpp      | 32 +++-----------------
 6 files changed, 82 insertions(+), 102 deletions(-)

I haven't investigated this further.
@einola einola added the bug Something isn't working label Oct 22, 2024
@einola
Copy link
Member Author

einola commented Oct 22, 2024

Link to the commit is here.

einola added a commit that referenced this issue Oct 22, 2024
@einola
Hotfix for issue #720
cbb2871 
I just reverted the offending comment and fixed a few formatting issues.
But this should be done properly at some point.
@einola einola mentioned this issue Oct 22, 2024
Merged
12 tasks
einola added a commit that referenced this issue Oct 23, 2024
@einola
Hotfix for issue #720 (#721)
c478cf4 
# Hotfix for bug in issue #720

Fixes partially #720

### Task List
- [x] Defined the tests that specify a complete and functioning change
(*It may help to create a [design specification & test
specification](../../../wiki/Specification-Template)*)
- [x] Implemented the source code change that satisfies the tests
- [x] Documented the feature by providing worked example
- [ ] Updated the README or other documentation
- [x] Completed the pre-Request checklist below

---
# Change Description

I just reverted the offending comment and fixed a few formatting issues.
But this should be done properly at some point.

---
# Test Description

The dynamics benchmark runs. A carefully placed break point or std::cout
confirms that the ``params`` class is properly set.

---
# Documentation Impact

N/A

---
# Other Details

N/A

---
### Pre-Request Checklist

- [x] The requirements of this pull request are fully captured in an
issue or design specification and are linked and summarised in the
description of this PR
- [x] No new warnings are generated
- [x] The documentation has been updated (or an issue has been created
to track the corresponding change)
- [x] Methods and Tests are commented such that they can be understood
without having to obtain additional context
- [x] This PR/Issue is labelled as a bug/feature/enhancement/breaking
change
- [x] File dates have been updated to reflect modification date
- [x] This change conforms to the conventions described in the README
@einola
Copy link
Member Author

einola commented Oct 25, 2024 •
edited
Loading

BBM still doesn't work, despite the hotfix.

@einola
Copy link
Member Author

einola commented Oct 25, 2024

There was a compile warning in develop before the hotfix; maybe this can help:

====================[ Build | nextsim | Debug ]=================================
/Applications/CLion.app/Contents/bin/cmake/mac/aarch64/bin/cmake --build /Users/einola/CLionProjects/nextsimdg/cmake-build-debug --target nextsim -j 6
[3/7] Building CXX object CMakeFiles/nextsimlib.dir/core/src/modules/DynamicsModule/module.cpp.o
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/module.cpp:12:
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/include/FreeDriftDynamics.hpp:11:
/Users/einola/CLionProjects/nextsimdg/dynamics/src/include/FreeDriftDynamicsKernel.hpp:39:70: warning: base class 'Nextsim::CGDynamicsKernel<6>' is uninitialized when used here to access 'Nextsim::CGDynamicsKernel<6>::u' [-Wuninitialized]
   39 |               sin(radians * paramsIn.ocean_turning_angle), paramsIn, u, v)
      |                                                                      ^
/Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/include/FreeDriftDynamics.hpp:30:11: note: in instantiation of member function 'Nextsim::FreeDriftDynamicsKernel<6>::FreeDriftDynamicsKernel' requested here
   30 |         , kernel(params)
      |           ^
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/module.cpp:12:
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/include/FreeDriftDynamics.hpp:11:
/Users/einola/CLionProjects/nextsimdg/dynamics/src/include/FreeDriftDynamicsKernel.hpp:39:73: warning: base class 'Nextsim::CGDynamicsKernel<6>' is uninitialized when used here to access 'Nextsim::CGDynamicsKernel<6>::v' [-Wuninitialized]
   39 |               sin(radians * paramsIn.ocean_turning_angle), paramsIn, u, v)
      |                                                                         ^
2 warnings generated.
[4/7] Building CXX object CMakeFiles/nextsimlib.dir/core/src/modules/DynamicsModule/MEVPDynamics.cpp.o
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/MEVPDynamics.cpp:10:
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/include/MEVPDynamics.hpp:15:
In file included from /Users/einola/CLionProjects/nextsimdg/dynamics/src/include/MEVPDynamicsKernel.hpp:12:
/Users/einola/CLionProjects/nextsimdg/dynamics/src/include/VPCGDynamicsKernel.hpp:58:59: warning: base class 'Nextsim::CGDynamicsKernel<6>' is uninitialized when used here to access 'Nextsim::CGDynamicsKernel<6>::u' [-Wuninitialized]
   58 |         : CGDynamicsKernel<DGadvection>(1., 0., paramsIn, u, v)
      |                                                           ^
/Users/einola/CLionProjects/nextsimdg/dynamics/src/include/MEVPDynamicsKernel.hpp:23:11: note: in instantiation of member function 'Nextsim::VPCGDynamicsKernel<6>::VPCGDynamicsKernel' requested here
   23 |         : VPCGDynamicsKernel<DGadvection>(MEVPStressStep, paramsIn)
      |           ^
/Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/MEVPDynamics.cpp:31:7: note: in instantiation of member function 'Nextsim::MEVPDynamicsKernel<6>::MEVPDynamicsKernel' requested here
   31 |     , kernel(params)
      |       ^
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/MEVPDynamics.cpp:10:
In file included from /Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/include/MEVPDynamics.hpp:15:
In file included from /Users/einola/CLionProjects/nextsimdg/dynamics/src/include/MEVPDynamicsKernel.hpp:12:
/Users/einola/CLionProjects/nextsimdg/dynamics/src/include/VPCGDynamicsKernel.hpp:58:62: warning: base class 'Nextsim::CGDynamicsKernel<6>' is uninitialized when used here to access 'Nextsim::CGDynamicsKernel<6>::v' [-Wuninitialized]
   58 |         : CGDynamicsKernel<DGadvection>(1., 0., paramsIn, u, v)
      |                                                              ^
2 warnings generated.
[7/7] Linking CXX executable nextsim

Build finished

@timspainNERSC
Copy link
Collaborator

I get the same warning in my branch where BBM definitely does work.

Add yes, we need to clean up those warnings.

@einola
Copy link
Member Author

einola commented Oct 25, 2024

I've been trying to figure this out, but without any luck. It does not destroy all the ice if I turn off the advection, but that's as far as I've gotten.

@timspainNERSC
Copy link
Collaborator

Oh!
Yes, I still need to debug the advection. That's why PR #668 is still a draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@einola einola

@timspainNERSC timspainNERSC

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@einola @timspainNERSC