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

H211b adaptive time-stepping #105

Open
RolfSander opened this issue Jun 27, 2024 · 9 comments
Open

H211b adaptive time-stepping #105

RolfSander opened this issue Jun 27, 2024 · 9 comments
Assignees
Labels
feature New feature or request integrators Related to numerical integrators
Milestone

Comments

@RolfSander
Copy link
Contributor

We already have the solver rosenbrock_posdef_h211b_qssa in the
user_contributed directory. Unfortunately, it has not been maintained
recently, and it hasn't worked with KPP3. Together with colleagues from
Jülich, I have now upgraded it so that it works out-of-the-box. We are
also writing a manuscript about the solver which should be ready soon.
The new code is in the branch
https://github.com/KineticPreProcessor/KPP/tree/feature/h211b. It
contains the new solver file int/rosenbrock_h211b_qssa.f90. You can
also find a simple test case in examples/mcm_h211b.

I hope that the code will be ready for a pull request soon. In the
meantime, please let me know if you have any comments or suggestions...

@yantosca yantosca added feature New feature or request integrators Related to numerical integrators labels Jun 27, 2024
@yantosca yantosca added this to the 3.2.0 milestone Jun 27, 2024
@yantosca
Copy link
Contributor

yantosca commented Jul 1, 2024

Thanks @RolfSander. I pushed a fix to the feature/h211b branch. Also am working on adding C-I tests.

@RolfSander
Copy link
Contributor Author

Oops. Thanks for the fix!

@yantosca
Copy link
Contributor

yantosca commented Jul 1, 2024

I also pushed some fixes for the examples & C-I tests. We now have C-I tests for mcm and `mcm_h211b. Here is a log of the output: ci_log.txt

@RolfSander
Copy link
Contributor Author

Thanks for adding the C-I tests!

I have a couple of comments/suggestions about the latest commit:

  • Thanks for creating the xrun scripts in both sh and csh syntax! Is
    there a reason why the new scripts don't set the environment variable
    KPP_HOME anymore?

  • The file constants_mcm.f90 has been downloaded from the MCM web page
    (https://mcm.york.ac.uk/MCM/export). For future compatibility, I'd
    like to change files from the MCM as little as possible. Specifically,
    I prefer to keep the name SUBROUTINE define_constants_mcm() instead
    of renaming it to SUBROUTINE define_constants().

  • In driver_mcm.f90, you changed USE constants_mcm to USE constants_KPP_ROOT. I think this should be reverted because
    constants_mcm.f90 is not a KPP-generated file. After reverting, the
    files mcm_isoprene.eqn and mcm_h211b_isoprene.eqn will be
    identical again.

@yantosca
Copy link
Contributor

yantosca commented Jul 2, 2024

Thanks @RolfSander. I can revert the changes. The problem that I was having is that the C-I tests were failing because the name of the folder is mcm_h211b but the KPP file is mcm.kpp. I can try to fix that so we don't change the MCM-related code that much.

@RolfSander
Copy link
Contributor Author

If the only requirement is that the *.kpp file has the same basename
as the directory, maybe it is sufficient to rename mcm_h211b/mcm.kpp
to mcm_h211b/mcm_h211b.kpp and keep the other files unchanged?

ps: It seems you've reverted everything now, including all the other
(very helpful) edits you've made. I hope it will be possible to
re-introduce them...

Copy link
Contributor

yantosca commented Jul 2, 2024

Thanks @RolfSander. I'll re-introduce what I did. Stay tuned.

Copy link
Contributor

yantosca commented Jul 2, 2024

Hi @RolfSander. I pushed some fixes. I edited the C-I test scripts so that you can specify not only the name of the folder but the name of the mechanism. This allows us to use mcm.kpp for both the MCM and MCM_H211b examples.

I also restored the run scripts (now named run_example.csh and run_example.sh) and made sure that the C-I tests build. Should be OK now.

@RolfSander
Copy link
Contributor Author

Thanks, this looks good now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request integrators Related to numerical integrators
Projects
None yet
Development

No branches or pull requests

2 participants