nextsimhub / nextsimdg

neXtSIM_DG : next generation sea-ice model with DG
https://nextsim-dg.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
10 stars 13 forks source link

Healing module #607

Closed einola closed 4 months ago

einola commented 4 months ago

Healing module

Fixes #529

Task List


Change Description

Implements a module to do damage healing. Implements a simple "constant healing" option, which is the default. Also implements a "no healing" option, which is used for mEVP (as mEVP doesn't have damage). Implementation of a temperature-dependent healing option is left to a separate issue.


Test Description

A unit test of the module has been implemented and runs as part of the CI. Damage healing also works with the dynamics benchmark when this is run with BBM.


Documentation Impact

The module includes on-line documentation.


Pre-Request Checklist

einola commented 4 months ago

The healing module doesn't actually produce help output with the --help-config option.

einola commented 4 months ago

I feel I should either add a null healing module or use the useDamage() function.

I tried adding if (Module::getInstance<IDynamics>()->usesDamage()) to line 166 of IceGrowth.cpp, but the model crashes. I am trying to understand why.

A null healing module is easy to do. It's not very useful for BBM, but if I can't get the useDamage() call to work, I can default to that for mEVP. The problem is that "nullHealing" makes sense as a default for mEVP but not BBM.

I'm still trying to figure out the best solution.

timspainNERSC commented 4 months ago

I feel I should either add a null healing module or use the useDamage() function.

I tried adding if (Module::getInstance<IDynamics>()->usesDamage()) to line 166 of IceGrowth.cpp, but the model crashes. I am trying to understand why.

A null healing module is easy to do. It's not very useful for BBM, but if I can't get the useDamage() call to work, I can default to that for mEVP. The problem is that "nullHealing" makes sense as a default for mEVP but not BBM.

I'm still trying to figure out the best solution.

The configuration of the DamageModule doesn't have to come from the configuration file. It can default to NullHealing, be set to ConstantHealing when BBMDynamics is configured, or let the configured module be set through the config file as with all the other modules. Perhaps something like

BBMDynamics::configure() {
…
 if (Module<IDamageHealing>.getName() == nullDamageName)
   Module<IDamageHealing>.setImplementation(constantDamageName);
…
}
einola commented 4 months ago

I like it ... but I want to do it slightly differently, of course ;)

ConstantHealing is default and using mEVP forces the use of NoHealing through the MEVP::configure function. I managed to get this to work and compile like this:

void MEVPDynamics::configure()
{
    Module::Module<IDamageHealing>().setImplementation("Nextsim::NoHealing");
}

but I get a lot of compile-time warnings:

====================[ Build | nextsim | Debug ]=================================
/Applications/CLion.app/Contents/bin/cmake/mac/aarch64/bin/cmake --build /Users/einola/CLionProjects/nextsimdg/cmake-build-debug --target nextsim -j 8
[37/87] 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:11:
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:56:9: warning: instantiation of variable 'Module::Module<Nextsim::IDamageHealing>::spf' required here, but no definition is available [-Wundefined-var-template]
        spf = generator;
        ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:64:13: note: in instantiation of member function 'Module::Module<Nextsim::IDamageHealing>::setExternalImplementation' requested here
            setExternalImplementation(functionMap.at(implName));
            ^
/Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/MEVPDynamics.cpp:26:38: note: in instantiation of member function 'Module::Module<Nextsim::IDamageHealing>::setImplementation' requested here
    Module::Module<IDamageHealing>().setImplementation("Nextsim::NoHealing");
                                     ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:101:15: note: forward declaration of template entity is here
    static fn spf;
              ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:56:9: note: add an explicit instantiation declaration to suppress this warning if 'Module::Module<Nextsim::IDamageHealing>::spf' is explicitly instantiated in another translation unit
        spf = generator;
        ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:57:9: warning: instantiation of variable 'Module::Module<Nextsim::IDamageHealing>::staticInstance' required here, but no definition is available [-Wundefined-var-template]
        staticInstance = std::move(spf());
        ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:102:31: note: forward declaration of template entity is here
    static std::unique_ptr<I> staticInstance;
                              ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:57:9: note: add an explicit instantiation declaration to suppress this warning if 'Module::Module<Nextsim::IDamageHealing>::staticInstance' is explicitly instantiated in another translation unit
        staticInstance = std::move(spf());
        ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:64:39: warning: instantiation of variable 'Module::Module<Nextsim::IDamageHealing>::functionMap' required here, but no definition is available [-Wundefined-var-template]
            setExternalImplementation(functionMap.at(implName));
                                      ^
/Users/einola/CLionProjects/nextsimdg/core/src/modules/DynamicsModule/MEVPDynamics.cpp:26:38: note: in instantiation of member function 'Module::Module<Nextsim::IDamageHealing>::setImplementation' requested here
    Module::Module<IDamageHealing>().setImplementation("Nextsim::NoHealing");
                                     ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:103:16: note: forward declaration of template entity is here
    static map functionMap;
               ^
/Users/einola/CLionProjects/nextsimdg/core/src/include/Module.hpp:64:39: note: add an explicit instantiation declaration to suppress this warning if 'Module::Module<Nextsim::IDamageHealing>::functionMap' is explicitly instantiated in another translation unit
            setExternalImplementation(functionMap.at(implName));
                                      ^
3 warnings generated.
[87/87] Linking CXX executable nextsim

Build finished

Any thoughts?

Also, I called it NoHealing instead of NullHealing. I think it's more descriptive. We can then have NoThermodynamics, NoDynamics, etc. But that's a different issue.

timspainNERSC commented 4 months ago

See the solution in PR #591:

solved by adding

extern template class Module::Module<ModularizedClassName>

before the class source code

einola commented 4 months ago

See the solution in PR #591:

solved by adding

extern template class Module::Module<ModularizedClassName>

before the class source code

Works like a charm :)