Skip to content

Add k.klaus - #153

Merged
jzwart merged 22 commits into
GLEON:mainfrom
bmcafee:main
Jul 8, 2026
Merged

Add k.klaus#153
jzwart merged 22 commits into
GLEON:mainfrom
bmcafee:main

Conversation

@bmcafee

@bmcafee bmcafee commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

This update adds the k.klaus and k.klaus.base functions. This k600 model is based on the work of Klaus and Vachon (2020, Aquatic Sciences). Per their paper, this model "is designed to fit average conditions across a wide range of global lakes". The implementation here does not account for the uncertainty included in the original model in order to be consistent with the other models in LakeMetabolizer. This uncertainty should ideally be added in a future update.

This update also constrains GPP and R in the metab.mle to be positive and negative, respectively. This was supposed to be a separate request but I forgot to make a separate branch. Too late now.
Edit: Not too late. All metab.mle changes are part of https://github.com/bmcafee/LakeMetabolizer2/tree/mleBootstrapping

@bmcafee bmcafee changed the title Add k.klaus Add k.klaus and constrain MLE estimates to correct sign Jun 9, 2026
@jzwart

jzwart commented Jul 7, 2026

Copy link
Copy Markdown
Member

As mentioned in email, let's focus this PR to just k.klaus and move the MLE constraint to its own PR (with other constraints where possible). Since all your MLE changes are just in R/metab.mle.R, the cleanest way is to revert just that file rather than rebasing commits:

# on your PR branch, with main synced in
git fetch upstream
git merge upstream/main        # picks up the merged tests (PR #152)
git checkout upstream/main -- R/metab.mle.R
git commit -m "Revert metab.mle changes; moving to separate PR"
git push

That leaves #153 as just k.klaus + k.read/NAMESPACE/man changes. Save the constrained metab.mle.R somewhere first (or grab it from this branch's history later) so it's easy to reopen as its own PR.

@jzwart

jzwart commented Jul 7, 2026

Copy link
Copy Markdown
Member

I'll review just the k.klaus updates

@jzwart

jzwart commented Jul 7, 2026

Copy link
Copy Markdown
Member

@MarcusKlaus could you take a look at the implementation of your gas exchange model in R/k.klaus.R in this PR? We are hoping to add it to the collection of models available in LakeMetabolizer

@bmcafee bmcafee changed the title Add k.klaus and constrain MLE estimates to correct sign Add k.klaus Jul 7, 2026
@bmcafee

bmcafee commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, @jzwart, the metab.mle changes have been removed. These will be reflected in a later PR. This PR now only has the k.klaus addition.

@jzwart jzwart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good @bmcafee . One change that is needed for functionality, and other changes are for consistency with other functions in lakemetabolizer.

You also might want to add a caveat that the authors do not recommend using the models with SDI in it - or the exp method for this new function. From the discussion section of that paper:

Some of the selected best candidate models included
SDI, suggesting the U10 effect on k600 to increase in lakes
with more complex shoreline geometry. This finding is
rather counter-intuitive as one would think that k600 should
decrease with SDI, given the increased shoreline sheltering
simulating the effect of a small lake (Schilder et al. 2013;
Vachon and Prairie 2013; Gålfalk et al. 2013). The positive
effect of SDI likely resulted from SDI being highly correlated
with LA (Online Resources Fig. S2A) and LA having a
positive effect on k600. We, therefore, conclude that sheltering
due to complex shorelines is not a dominant modulator
of the U10-k600 relationship and do not recommend using our
models that include SDI.

Marcus might have an opinion on if it should even be included in the code implementaiton - I sent him an email and got a response that he's out of the office until the beginning of August. Based on the text from the paper, you might want to just remove the SDI / exp option altogether.

Comment thread R/k.klaus.R Outdated
Comment thread R/k.klaus.R Outdated
Comment thread R/k.klaus.R Outdated
Comment thread R/k.klaus.R Outdated
Comment thread R/k.klaus.R Outdated
Comment thread R/k.read.R
Comment thread R/k.read.R
Comment thread R/k.read.R Outdated
Comment thread R/k.read.R Outdated
bmcafee and others added 10 commits July 8, 2026 08:53
Co-authored-by: Jake Zwart <jayzlimno@gmail.com>
Co-authored-by: Jake Zwart <jayzlimno@gmail.com>
Co-authored-by: Jake Zwart <jayzlimno@gmail.com>
Co-authored-by: Jake Zwart <jayzlimno@gmail.com>
Co-authored-by: Jake Zwart <jayzlimno@gmail.com>
Co-authored-by: Jake Zwart <jayzlimno@gmail.com>
@jzwart
jzwart self-requested a review July 8, 2026 17:37

@jzwart jzwart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! thanks for making the changes.

@jzwart
jzwart merged commit bc84930 into GLEON:main Jul 8, 2026
5 checks passed
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.

2 participants