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

Corrected unit of q_stc to pA in gif_psc_exp #3405

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Laxmi-gupta
Copy link

This is my first contribution, and I noticed that the adaptive current q_stc was mentioned in nA, but it should be in pA.

I made two changes:

Updated the comment to correctly state pA instead of nA.
Converted the value by multiplying it by 1000 to properly reflect pA in the calculation.

Changes made in:
File: models/gif_psc_exp.cpp
Line: 357

I haven’t tested this yet since it’s my first issue, but I followed the logic and made the necessary correction.

@clinssen
Copy link
Contributor

Hi, thank you for your contribution!

You mention that q_stc "should be" in pA. Could you say why this is so? Defining it in pA and multiplying it with a factor 1E3 is equivalent and so does not really fix a bug per se; it is a matter of convention (which could certainly be a legitimate reason, just wondering if you have a particular reason or reference for this). Cheers!

By the way, to run the testsuite locally, you can run make installcheck from the build directory.

@Laxmi-gupta
Copy link
Author

Thank you for your feedback and for pointing out the equivalence between defining q_stc in nA and pA. You're absolutely correct that mathematically, defining q_stc in nA and scaling it by 1e3 is equivalent to defining it directly in pA. However, this change is primarily rooted in consistency and clarity within the model.
Thank you also for the tip about running the testsuite locally with make installcheck—I’ll definitely make use of that.

@clinssen
Copy link
Contributor

Could you say in more detail what the consistency is between? There are many variants of the "gif" model that also define q_stc in nA, so to keep it consistent, they would all need to be changed. Could you give a reference to an equation or parameters table in the literature (like to the original publication of the model) where the value is given in pA? Much obliged!

@gtrensch gtrensch added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

3 participants