Dissolution, physical erosion and emperical denudation impletement

[xyl96]

Adding new features including chemical dissolution, physical erosion and emperical denudation into the main branch. Unit test is not included and will be included afterwards. Please check it and we can discuss whether these codes meet the goals set in the documentation.

[EmiliaJarochowska]

I would like make some corrections related to the documentation and its agreement with the code, but this branch does not have the corrections that had been made to main to run entangled so I have issues running it. What's the best way to apply these corrections here? I cannot pull main here because of conflicts, should I copy Poetry.lock here by hand? @jhidding @HannoSpreeuw

[jhidding]

You could try git checkout main pyproject.toml poetry.lock.

[EmiliaJarochowska]

Oh I didn't realize I could checkout individual files from a branch. Thanks.

[EmiliaJarochowska]

After updating the branch with a venv for Entangled, which should help make problems reproducible, I get:

[09:58:58] WARNING  File `src/Erosion.jl` in DB doesn't exist. Removing entry
                    from DB.
           INFO     orphans found:
                    `.entangled/build/unnamed-docs/src/stencils.md_174, 
                    .entangled/build/unnamed-docs/src/stencils.md_217, 
                    .entangled/build/unnamed-docs/src/stencils.md_134`
           INFO     create `src/Erosion2.jl`
           INFO     create `src/Erosion.jl`
           INFO     create `src/CarbDissolution.jl`
           INFO     create `src/EmpericalDenudation.jl`
           INFO     create `.entangled/tasks.json`
           INFO     delete `.entangled/build/unnamed-docs/src/stencils.md_174`
           INFO     delete `.entangled/build/unnamed-docs/src/stencils.md_217`
           INFO     delete `.entangled/build/unnamed-docs/src/stencils.md_134`
           WARNING  src/CarbDissolution.jl is not managed by Entangled
           WARNING  src/EmpericalDenudation.jl is not managed by Entangled
           ERROR    conflicts found, breaking off (use `--force` to run anyway)

What does it mean "is not managed by Entangled"? Should it not be added to the database automatically? I cannot find anything about that in Entangled manual.

[jhidding]

Hi @xyl96,

I talked @EmiliaJarochowska today and fixed some immediate issues with this PR. Others we still need to discuss.

How-to write modules

For the moment I copy-pasted the original model code into new files. On the longer term we'd need to de-duplicate the codes to get better reuse, but that is only something we can do in an informed manner once we have more physics modules in place (i.e. including transport). As a general rule, major updates like yours should be done in such a way, that they don't break existing code (superseding the rule of trying not to repeat code).

This also means your code will go into a separate namespace of CarboKitten:

• Generic physics involving erosion will go into CarboKitten.Erosion.
• Model runs that include CA, production and erosion go into CarboKitten.CaProdErosion. (Until we find a better name)

This way we can reuse the stuff in CarboKitten.Erosion while keeping the rest of the code base operational. While I'm typing this, I think we should rename the CaProd, CaProdErosion and CAPT (from the transport branch) to CarboKitten.Run.*.

Type-level dispatch in Julia

You have four different types of Erosion specified. Then, inside the code there is a big if-statement that dispatches to different implementations of erosion. There are two things we can remark about this. The integers that encode for different types of erosion should be self-documenting values. In general you'd use an Enum for that (enums are a concept in most programming languages), letting you put names to individual integer values.

In Julia however, the entire if-statement can be gotten rid of by using the function-dispatch mechanism. This comes with a bit of a warning: overuse of this mechanism increases compile times (see: this bit on Julia performance. In our case, we run the entire model with one choice for erosion model, so using type level dispatch seems the correct choice. As an example (not sure if it will be exactly like this),

abstract type DenudationType end

struct NoDenudationType <: DenudationType end

const NoDenudation = NoDenudationType()

struct Dissolution <: DenudationType
  temp::Float64
  precip::Float64
  alpha::Float64
  pco2::Float64
end

function denudation(::DT, water_depth::Float64, facies::Facies) where {DT <: DenudationType}
  error("Unknown denudation method specified")
end

function denudation(::NoDenudationType, water_depth::Float64, facies::Facies)
  return water_depth
end

function denudation(d::Dissolution, water_depth::Float64, facies::Facies)
  return dissolution(d.temp, d.precip, d.alpha, d.pco2, water_depth, facies)
end

Then in the model run code we may call the denudation function with the type and values given in the Input.

input = Input(
   ...
   # denudation = NoDenudation,
   denudation = Dissolution(
     temp = 288.0u"K",
     precip = 1000.0u"mm/yr",
     pco2 = 10^(-1.5),   # I don't know the units
     alpha = 2e-3,
   )
   ...
)

CarboKitten.Run.SomethingWithErosion(input)

[jhidding]

Prep for @jhidding:

• [x] solve issue #2
• [ ] merge feature-units branch

TODO:

• [x] we should rename everything from Erosion to Denudation
• [ ] group modules in one Denudation parent module
• [ ] tidy up documentation and description of the physics
• [ ] add proper references to docs/src/ref.bib

[HannoSpreeuw]

I am trying to get on track with the features from the xy_test branch.

Why does include("examples/ca-with-prod.jl") give this error:

ERROR: LoadError: MethodError: no method matching CarboKitten.Burgess2013.Config.Facies(::Tuple{Int64, Int64}, ::Tuple{Int64, Int64}, ::Float64, ::Float64, ::Int64)

Closest candidates are:
  CarboKitten.Burgess2013.Config.Facies(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any)
   @ CarboKitten ../../CarboKitten.jl/src/Burgess2013/Config.jl:7
  CarboKitten.Burgess2013.Config.Facies(::Tuple{Int64, Int64}, ::Tuple{Int64, Int64}, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64)
   @ CarboKitten ../../CarboKitten.jl/src/Burgess2013/Config.jl:7

[EmiliaJarochowska]

Well spotted... @xyl96 added elements to the Facies struct

    L::Float64 #reactive surface
    density::Float64 #density of different carb factory
    inf::Float64 #infiltration coeff

but these parameters are only needed when using the Denudation module and should not be required by default, so examples written for the main don't have these parameters. And adding them breaks down the compatibility with already existing examples. Probably just make them optional parameters? Or is there a more elegant way?

[HannoSpreeuw]

@xyl96 Wants to add the denudation as a separate module to this branch.

[EmiliaJarochowska]

I cannot fully assess the changes because I messed up Makie and cannot plot (ERROR: LoadError: MethodError: no method matching gl_convert(::Makie.DimConversions)) and I can't fix it at the moment but I noted some obvious things that need attention:

1. Documentation is not up to date, it needs clear instructions on how to use the module
2. Notebooks (e.g this) are not up to date and won't work with the current version od the code
3. Conflicts with the tests need to be resolved The rest we clarify in person.

[EmiliaJarochowska]

We discussed this PR with @xyl96 today. What remains to be done is:

• [ ] updating the documentation
• [ ] making an example illustrating the functioning of all three algorithms
• [ ] adding a regression test based on small matrix of facies The current tests look very good :-) @HannoSpreeuw do you want to be a reviewer on this PR or is this still too early in your acquaintance with CarboKitten?