Skip to content

[SPH] fix typo in monofluid naming#1911

Open
tdavidcl wants to merge 1 commit into
Shamrock-code:mainfrom
tdavidcl:monofluid_typo
Open

[SPH] fix typo in monofluid naming#1911
tdavidcl wants to merge 1 commit into
Shamrock-code:mainfrom
tdavidcl:monofluid_typo

Conversation

@tdavidcl

@tdavidcl tdavidcl commented Jul 9, 2026

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
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 gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 renames the monofluid dust mode from TVI to TVA across C++ configurations, Python bindings, and example scripts. To ensure backward compatibility, the feedback suggests supporting the old 'monofluid_tvi' type during JSON deserialization of configuration files and retaining a deprecated 'set_dust_mode_monofluid_tvi' alias in the Python bindings to avoid breaking existing user scripts.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +164 to 166
} else if (type == "monofluid_tva") {
set_monofluid_tva(
j.at("ndust").get<u32>(), j.at("pure_diffusion_mode").get<bool>());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To maintain backward compatibility with existing simulation configuration files and checkpoint dumps that contain "type": "monofluid_tvi", we should also accept "monofluid_tvi" and map it to set_monofluid_tva during JSON deserialization.

Suggested change
} else if (type == "monofluid_tva") {
set_monofluid_tva(
j.at("ndust").get<u32>(), j.at("pure_diffusion_mode").get<bool>());
} else if (type == "monofluid_tva" || type == "monofluid_tvi") {
set_monofluid_tva(
j.at("ndust").get<u32>(), j.at("pure_diffusion_mode").get<bool>());

Comment on lines 268 to 272
.def(
"set_dust_mode_monofluid_tvi",
"set_dust_mode_monofluid_tva",
[](TConfig &self, u32 nvar, bool pure_diffusion_mode) {
self.dust_config.set_monofluid_tvi(nvar, pure_diffusion_mode);
self.dust_config.set_monofluid_tva(nvar, pure_diffusion_mode);
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To prevent breaking existing Python scripts that call set_dust_mode_monofluid_tvi, we should keep a deprecated alias in the Python bindings that prints a warning and delegates to the new set_dust_mode_monofluid_tva method.

        .def(
            "set_dust_mode_monofluid_tva",
            [](TConfig &self, u32 nvar, bool pure_diffusion_mode) {
                self.dust_config.set_monofluid_tva(nvar, pure_diffusion_mode);
            },
            py::kw_only(),
            py::arg("nvar"),
            py::arg("pure_diffusion_mode") = false)
        .def(
            "set_dust_mode_monofluid_tvi",
            [](TConfig &self, u32 nvar, bool pure_diffusion_mode) {
                ON_RANK_0(shamlog_warn_ln(
                              "SPH",
                              ".set_dust_mode_monofluid_tvi() is deprecated, please use .set_dust_mode_monofluid_tva() instead."));
                self.dust_config.set_monofluid_tva(nvar, pure_diffusion_mode);
            },

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Workflow report

workflow report corresponding to commit 92ed469
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
cmake-format.............................................................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.

Clang-tidy diff report


600 warnings generated.
Suppressed 601 warnings (598 in non-user code, 2 due to line filter, 1 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

1136 warnings generated.
Suppressed 1137 warnings (1038 in non-user code, 98 due to line filter, 1 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Doxygen diff with main

Removed warnings : 5
New warnings : 5
Warnings count : 8006 → 8006 (0.0%)

Detailed changes :
+ src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:118: warning: Compound shammodels::sph::DustConfig::MonofluidTVA is not documented.
- src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:118: warning: Compound shammodels::sph::DustConfig::MonofluidTVI is not documented.
+ src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:119: warning: Member ndust (variable) of struct shammodels::sph::DustConfig::MonofluidTVA is not documented.
- src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:119: warning: Member ndust (variable) of struct shammodels::sph::DustConfig::MonofluidTVI is not documented.
+ src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:120: warning: Member pure_diffusion_mode (variable) of struct shammodels::sph::DustConfig::MonofluidTVA is not documented.
- src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:120: warning: Member pure_diffusion_mode (variable) of struct shammodels::sph::DustConfig::MonofluidTVI is not documented.
+ src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:133: warning: Member set_monofluid_tva(u32 nvar, bool pure_diffusion_mode=false) (function) of struct shammodels::sph::DustConfig is not documented.
- src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:133: warning: Member set_monofluid_tvi(u32 nvar, bool pure_diffusion_mode=false) (function) of struct shammodels::sph::DustConfig is not documented.
+ src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:139: warning: Member is_monofluid_tva() (function) of struct shammodels::sph::DustConfig is not documented.
- src/shammodels/sph/include/shammodels/sph/SolverConfig.hpp:139: warning: Member is_monofluid_tvi() (function) of struct shammodels::sph::DustConfig is not documented.

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.

1 participant