Skip to content

Conversation

hyeoksu-lee
Copy link
Contributor

@hyeoksu-lee hyeoksu-lee commented Jul 25, 2025

User description

Description

I noticed that the example 0D_bubblecollapse_adap is broken. The reason was that when Lagrange bubble model was updated, the maximum iteration for adaptive time stepping was fixed to 100, while this case requires more than 100 as it involves violent collapse of bubbles. Therefore, I moved adap_dt_max_iters from m_constant to user input parameter.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Scope

  • This PR comprises a set of related changes with a common goal

How Has This Been Tested?

The figure shows the comparison between adap_dt_max_iters = 100 and adap_dt_max_iters = 200. For adap_dt_max_iters = 100, the rebound cannot be captured.
results

Test Configuration:

  • What computers and compilers did you use to test this: MacBook M4 Pro

Checklist

  • I have added comments for the new code
  • I added Doxygen docstrings to the new code
  • I have made corresponding changes to the documentation (docs/)

PR Type

Bug fix


Description

  • Move adap_dt_max_iters from constant to user parameter

  • Fix broken 0D bubble collapse adaptive example

  • Add documentation for new adaptive time stepping parameters


Diagram Walkthrough

flowchart LR
  A["m_constants.fpp"] -- "remove constant" --> B["m_global_parameters.fpp"]
  B -- "add user parameter" --> C["case.py"]
  C -- "set adap_dt_max_iters=200" --> D["Fixed Example"]
  B -- "parameter propagation" --> E["m_mpi_proxy.fpp"]
  B -- "parameter propagation" --> F["m_start_up.fpp"]
  G["case_dicts.py"] -- "add parameter type" --> H["Documentation"]
Loading

File Walkthrough

Relevant files

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Default Value

The default value of 100 for adap_dt_max_iters is set in the initialization function, but this was the original problematic value that caused the bug. Consider if this default should be higher to prevent similar issues in other cases.

adap_dt_max_iters = 100
Missing Tests

The PR fixes a simulation feature but does not include any automated tests to verify the fix works correctly or to prevent regression of this issue in the future.

"adap_dt_max_iters": 200,
# Gas compression model

Copy link

qodo-merge-pro bot commented Jul 25, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use constant instead of hardcoded value
Suggestion Impact:The commit directly implements the suggestion by replacing the hardcoded value 100 with the constant dflt_adap_dt_max_iters

code diff:

-        adap_dt_max_iters = 100
+        adap_dt_max_iters = dflt_adap_dt_max_iters

The hardcoded value should use the default constant from m_constants module for
consistency with other default parameter assignments. This ensures centralized
configuration management and easier maintenance.

src/simulation/m_global_parameters.fpp [680]

-adap_dt_max_iters = 100
+adap_dt_max_iters = dflt_adap_dt_max_iters
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly identifies the use of a magic number (100) and recommends using a named constant, which is a significant improvement for code readability and maintainability.

Medium
Add missing default constant parameter
Suggestion Impact:The suggestion was directly implemented - the default constant dflt_adap_dt_max_iters was added with value 100 and appropriate comment

code diff:

+    integer, parameter :: dflt_adap_dt_max_iters = 100 !< Default max iteration for adaptive step size

The removal of adap_dt_max_iters constant leaves a gap in the constants section.
Since this parameter is now moved to be a configurable variable, consider adding
a default constant for consistency with the pattern used for dflt_adap_dt_tol.

src/common/m_constants.fpp [57-59]

 real(wp), parameter :: dflt_adap_dt_tol = 1.e-4_wp !< Default tolerance for adaptive step size
-
+integer, parameter :: dflt_adap_dt_max_iters = 100 !< Default maximum number of iterations for adaptive step size
 ! Constants of the algorithm described by Heirer, E. Hairer S.P.Nørsett G. Wanner, Solving Ordinary Differential Equations I, Chapter II.4
Suggestion importance[1-10]: 6

__

Why: This suggestion correctly points out that a default constant dflt_adap_dt_max_iters should be added for consistency with dflt_adap_dt_tol, improving maintainability by centralizing default values.

Low
  • Update

Copy link

codecov bot commented Jul 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.04%. Comparing base (ada47da) to head (55dc6b4).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #969   +/-   ##
=======================================
  Coverage   44.04%   44.04%           
=======================================
  Files          69       69           
  Lines       19611    19612    +1     
  Branches     2430     2430           
=======================================
+ Hits         8637     8638    +1     
  Misses       9474     9474           
  Partials     1500     1500           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbryngelson sbryngelson merged commit 16de11c into MFlowCode:master Jul 26, 2025
30 checks passed
@hyeoksu-lee hyeoksu-lee deleted the adap_dt_max branch July 26, 2025 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants