Skip to content

[Ramses] add a string based slope limiter selection#1637

Open
tdavidcl wants to merge 4 commits intoShamrock-code:mainfrom
tdavidcl:patch-2026-02-12-13-46
Open

[Ramses] add a string based slope limiter selection#1637
tdavidcl wants to merge 4 commits intoShamrock-code:mainfrom
tdavidcl:patch-2026-02-12-13-46

Conversation

@tdavidcl
Copy link
Member

No description provided.

@github-actions
Copy link
Contributor

Thanks @tdavidcl for opening this PR!

You can do multiple things directly here:
1 - Comment pre-commit.ci run to run pre-commit checks.
2 - Comment pre-commit.ci autofix to apply fixes.
3 - Add label autofix.ci to fix authorship & pre-commit for every commit made.
4 - Add label light-ci to only trigger a reduced & faster version of the CI (need the full one before merge).
5 - Add label trigger-ci to create an empty commit to trigger the CI.

Once the workflow completes a message will appear displaying informations related to the run.

Also the PR gets automatically reviewed by gemini, you can:
1 - Comment /gemini review to trigger a review
2 - Comment /gemini summary for a summary
3 - Tag it using @gemini-code-assist either in the PR or in review comments on files

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @tdavidcl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the mechanism for configuring slope limiters within the Ramses solver. It consolidates multiple discrete setter functions into a single, string-based method, enhancing the API's consistency and future extensibility. This change simplifies how users specify slope limiters and includes robust error handling for invalid selections.

Highlights

  • Unified Slope Limiter Selection: A new set_slope_limiter method has been introduced in SolverConfig.hpp that allows selecting the slope limiter mode using a string argument, providing a more flexible and extensible API.
  • Deprecated Individual Setters: The previous individual set_slope_lim_... methods in the Python binding (pyRamsesModel.cpp) have been deprecated. They now issue warning messages and internally call the new string-based set_slope_limiter method.
  • Example Update: An example script (sod_tube_godunov_amr.py) has been updated to demonstrate the usage of the new string-based set_slope_limiter method.
Changelog
  • doc/sphinx/examples/tests_ci/sod_tube_godunov_amr.py
    • Updated the example to use cfg.set_slope_limiter("minmod") instead of the deprecated cfg.set_slope_lim_minmod().
  • src/shammodels/ramses/include/shammodels/ramses/SolverConfig.hpp
    • Added a std::map named slope_mode_string_map to associate string representations with SlopeMode enum values.
    • Implemented a static helper function get_valid_slope_modes to return a vector of supported slope limiter strings.
    • Introduced the set_slope_limiter method, which takes a string, maps it to a SlopeMode, and sets slope_config, throwing an std::invalid_argument for unsupported strings.
  • src/shammodels/ramses/src/pyRamsesModel.cpp
    • Exposed the new set_slope_limiter method to the Python API with appropriate arguments and documentation.
    • Modified all existing set_slope_lim_none, set_slope_lim_vanleer_f, set_slope_lim_vanleer_std, set_slope_lim_vanleer_sym, and set_slope_lim_minmod Python binding functions to issue deprecation warnings and call the new set_slope_limiter internally.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a more user-friendly, string-based method for selecting the slope limiter, deprecating the old individual setter methods. This is a good API improvement. The implementation is well-executed, including proper error handling for invalid inputs and clear deprecation warnings in the Python bindings. My review includes a suggestion to optimize the error reporting mechanism for better efficiency.

Copy link
Collaborator

@nbrucy nbrucy left a comment

Choose a reason for hiding this comment

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

It's a good first step on how to "deverbosify" a parameter.

As we discussed, in the long term a unified "matplotlib rcParams" mechanism could be nice (see #1651).

@tdavidcl
Copy link
Member Author

i quite like this approach too, i only want to answer that question before merging it #1651 (comment)

tdavidcl and others added 2 commits February 16, 2026 23:19
Co-authored-by: Noé Brucy <nbrucy@users.noreply.github.com>
@github-actions
Copy link
Contributor

Workflow report

workflow report corresponding to commit 0d3f31a
Commiter email is timothee.davidcleris@proton.me

Pre-commit check report

Pre-commit check: ✅

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check for merge conflicts................................................Passed
check that executables have shebangs.....................................Passed
check that scripts with shebangs are executable..........................Passed
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check for broken symlinks................................................Passed
check yaml...............................................................Passed
detect private key.......................................................Passed
No-tabs checker..........................................................Passed
Tabs remover.............................................................Passed
Validate GitHub Workflows................................................Passed
clang-format.............................................................Passed
ruff check...............................................................Passed
ruff format..............................................................Passed
Check doxygen headers....................................................Passed
Check license headers....................................................Passed
Check #pragma once.......................................................Passed
Check SYCL #include......................................................Passed
No ssh in git submodules remote..........................................Passed
No UTF-8 in files (except for authors)...................................Passed

Test pipeline can run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants