Precommit test by ykempf · Pull Request #1182 · fmihpc/vlasiator

ykempf · 2025-09-24T13:10:27Z

No description provided.

ursg

I only took some picks of files here and there (and did not scroll through 280 files in 10 minutes!). There are definitely some clang-format options that still need tuning.

ursg · 2025-09-25T08:49:45Z

      uint thread_id = 0;

   public:
+      void syncDeviceData(void) { CHK_ERR(cudaMemcpyAsync(d_ptr, ptr, bytes, cudaMemcpyHostToDevice, gpuStreamList[thread_id])); }


I don't like that these have been compressed to one-liners now. I wonder what clang-format option would help there?

I guess AllowShortFunctionsOnASingleLine=Empty would be a reasonable setting for this?

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortblocksonasingleline

ursg · 2025-09-25T08:51:37Z

+      void setDipoleField(const FieldFunction& dipole) { dipoleField = dipole; };
+      void setConstantBackgroundField(const std::array<Real, 3> B) { BGB = B; }


Here, too: The threshold for single-lining simple functions is too small, IMHO

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortblocksonasingleline

ursg · 2025-09-25T08:52:11Z

-      void solve(
-         int & iteration,
-         int & nRestarts,
-         Real & residual,
-         Real & minPotentialN,
-         Real & maxPotentialN,
-         Real & minPotentialS,
-         Real & maxPotentialS
-      );
-      void solveInternal(
-         int & iteration,
-         int & nRestarts,
-         Real & residual,
-         Real & minPotentialN,
-         Real & maxPotentialN,
-         Real & minPotentialS,
-         Real & maxPotentialS
-      );
+      void initSolver(bool zeroOut = true);                                      /*!< Initialize the CG solver */
+      iSolverReal Atimes(uint nodeIndex, int parameter, bool transpose = false); /*!< Evaluate neighbour nodes' coupled parameter */
+      Real Asolve(uint nodeIndex, int parameter, bool transpose = false);        /*!< Evaluate own parameter value */
+      void solve(int& iteration, int& nRestarts, Real& residual, Real& minPotentialN, Real& maxPotentialN, Real& minPotentialS, Real& maxPotentialS);
+      void solveInternal(int& iteration, int& nRestarts, Real& residual, Real& minPotentialN, Real& maxPotentialN, Real& minPotentialS, Real& maxPotentialS);


These, too, are less readable now than before.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#binpackarguments

This might be a consequence of having too permissive line width, I'm assuming it's only going to split calls and definitions if they don't fit

This and the below are better imho if forcing them to just have one per line in a blanket fashion. Having variably one or more per line is just going to confuse people.

Yeah BPPS_AlwaysOnePerLine is probably fine if not ideal imo. I don't think I agree about it being case-by-case being bad, like 1-3 param functions can be single-line. How does the formatting look with a lower line width?

Well, this turned out to be absolutely awful as it applies the same rule to template parameters which we absolutely do not want one per line, ever. So we'll have to live with BPPS_OnePerLine which will occasionally result in these one-liners (we'd have to tone down line width to address that)

ursg · 2025-09-25T08:52:51Z

+      void mapDownBoundaryData(FsGrid<std::array<Real, fsgrids::bfield::N_BFIELD>, FS_STENCIL_WIDTH>& perBGrid, FsGrid<std::array<Real, fsgrids::dperb::N_DPERB>, FS_STENCIL_WIDTH>& dPerBGrid,
+                               FsGrid<std::array<Real, fsgrids::moments::N_MOMENTS>, FS_STENCIL_WIDTH>& momentsGrid, FsGrid<std::array<Real, fsgrids::volfields::N_VOL>, FS_STENCIL_WIDTH>& volGrid,
+                               FsGrid<fsgrids::technical, FS_STENCIL_WIDTH>& technicalGrid);


Wasn't this exactly what the BinPackArguments option was supposed to prevent? Or is this now only affecting arguments in the function definition, and is separately handled for declarations?

Ah, it is BinPackArguments (which are the actual values supplied in a function invocation) vs. BinPackParameters (which is the parameter list in the function definition and/or declaration.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#binpackparameters

There is also "ExperimentalAutoDetectBinPacking", but I have no idea what it does.

ursg · 2025-09-25T08:54:28Z

+      for (auto& c : lowercase)
+         c = tolower(c);


This should definitely have curlies.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#insertbraces

ursg · 2025-09-25T08:55:42Z

The formatting of all these datareducers, that I had been quite vocal about in the fsgrid PR, now actually looks pretty nice!

ursg · 2025-09-25T09:08:29Z

scrolling through the config reference, i see ForEachMacros, that tells clang-format which custom macros should be treated as for loops. Our arch looping mechanisms are candidates for that.

ykempf · 2025-09-25T10:14:47Z

In other news, we have a successful pair of TP runs! 🎉

lkotipal · 2025-09-25T13:14:24Z

 bool writeGrid(
-   dccrg::Dccrg<SpatialCell,dccrg::Cartesian_Geometry>& mpiGrid,
-   FsGrid< std::array<Real, fsgrids::bfield::N_BFIELD>, FS_STENCIL_WIDTH> & perBGrid,
-   FsGrid< std::array<Real, fsgrids::efield::N_EFIELD>, FS_STENCIL_WIDTH> & EGrid,
-   FsGrid< std::array<Real, fsgrids::ehall::N_EHALL>, FS_STENCIL_WIDTH> & EHallGrid,
-   FsGrid< std::array<Real, fsgrids::egradpe::N_EGRADPE>, FS_STENCIL_WIDTH> & EGradPeGrid,
-   FsGrid< std::array<Real, fsgrids::moments::N_MOMENTS>, FS_STENCIL_WIDTH> & momentsGrid,
-   FsGrid< std::array<Real, fsgrids::dperb::N_DPERB>, FS_STENCIL_WIDTH> & dPerBGrid,
-   FsGrid< std::array<Real, fsgrids::dmoments::N_DMOMENTS>, FS_STENCIL_WIDTH> & dMomentsGrid,
-   FsGrid< std::array<Real, fsgrids::bgbfield::N_BGB>, FS_STENCIL_WIDTH> & BgBGrid,
-   FsGrid< std::array<Real, fsgrids::volfields::N_VOL>, FS_STENCIL_WIDTH> & volGrid,
-   FsGrid< fsgrids::technical, FS_STENCIL_WIDTH> & technicalGrid,
+   dccrg::Dccrg<
+      SpatialCell,
+      dccrg::Cartesian_Geometry>& mpiGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::bfield::N_BFIELD>,
+      FS_STENCIL_WIDTH>& perBGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::efield::N_EFIELD>,
+      FS_STENCIL_WIDTH>& EGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::ehall::N_EHALL>,
+      FS_STENCIL_WIDTH>& EHallGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::egradpe::N_EGRADPE>,
+      FS_STENCIL_WIDTH>& EGradPeGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::moments::N_MOMENTS>,
+      FS_STENCIL_WIDTH>& momentsGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::dperb::N_DPERB>,
+      FS_STENCIL_WIDTH>& dPerBGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::dmoments::N_DMOMENTS>,
+      FS_STENCIL_WIDTH>& dMomentsGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::bgbfield::N_BGB>,
+      FS_STENCIL_WIDTH>& BgBGrid,
+   FsGrid<
+      std::array<
+         Real,
+         fsgrids::volfields::N_VOL>,
+      FS_STENCIL_WIDTH>& volGrid,
+   FsGrid<
+      fsgrids::technical,
+      FS_STENCIL_WIDTH>& technicalGrid,


Oh my god bruh

Tbh this particular mess could do with a proper fsgrids wrapper anyhow.

Actually include clang-format 22 in CI step (Re-)Enable clang-format in precommit config Actually run on push to precommitTest Fix precommit action yml file Fix precommit ci apt installation commands Also install python in precommit CI try to get precommit to actually find git Make sure precommit runs in the correct directory Try to debug why git is not found in precommit CI See if including npm or nodejs help widg precommit artefact upload Don't actually run clang-format (but everything else) Make sure precommit ci step has the right name Re-enable clang-format Actually add push permissions to the precommit workflow Also ignore .DAT files and any line with a # in it. Actually, *don't* bin pack argument Update some precommit hook versions Run precommit CI on push to any branch (thus: permission!) Actually have pull-request, not push permissions for precommit A random one-character commit that introduces a whitespace error. Remove the whitespace error again. Fix order of filter. swap order because why not? Whyyyy? Add back precommitTest to change the PR target branch again. No branch specification, run that on any PR. Another unimportant small change More trivial and pointless change Add a poem about neutrinos.

ursg · 2025-10-29T11:41:17Z

+#define CHECK_FLOAT(x)                                                                                                                                                                                 \
+   {                                                                                                                                                                                                   \
+   }


This is a weird way to reformat this. It seems to identify the "{}" here as an empty code block, but this totally breaks the ideomatic use of {} to signify an empty piece of code.

Also, why does it even touch this line, if the regex forbids it from touching anything with a # in it?

yyyaaammmlll......

reverting some Revert "reverting some" This reverts commit 13242b0. Finally?

one more space Y and Z clang format bypass in Hall term too.

ursg · 2025-10-29T13:46:18Z

To sum up our findings of today:
a) By and large, the formatting is now looking good.
b) We manually disabled clang-format for some code blocks that are semantically grouped for ease of understanding
c) In principle, we are now happy and would merge it, but it looks like we are triggering a clang-format bug with our regex-ignoring of pragmas: many (all?) comment lines preceding a pragma are alternatingly moved to column one and back to their proper indentation on every even-odd run of clang-format.

Do we have a workaround for that?

ursg · 2025-10-30T07:46:59Z

So your solution is to have a "#" in every comment in front of an omp pragma, hence clang-format ignoring it?
And if we forget that in the future, precommit will hapilly mess up our code again?

"No sir, I don't like it."

ykempf force-pushed the precommitTest branch 9 times, most recently from d4375f9 to 9a527f2 Compare September 25, 2025 08:29

ursg requested changes Sep 25, 2025

View reviewed changes

ykempf force-pushed the precommitTest branch 9 times, most recently from d1bef79 to 573af76 Compare September 25, 2025 13:09

lkotipal reviewed Sep 25, 2025

View reviewed changes

ykempf force-pushed the precommitTest branch from bacbef1 to 191cdb3 Compare September 25, 2025 16:40

ursg and others added 7 commits October 29, 2025 13:51

Try checking out PR HEAD instead of merge commit

3824498

Now trying with pull_request_target instead

339cb43

Update precommit.yml

4461db0

Manually fix whitespace in testpackage reference yaml

0539b2e

Rename miniapp common.h

414b881

new line at file end

59af9f5

ykempf added 3 commits October 29, 2025 13:52

fix yaml comment

c3b2fea

Removed empty line

af568f7

Sanitize .clang-format even more...

8fd287d

ursg force-pushed the precommitTest branch from 7e884f5 to 914388d Compare October 29, 2025 11:53

ursg reviewed Oct 29, 2025

View reviewed changes

ykempf and others added 7 commits October 29, 2025 15:03

--- ... in that yaml file

a18f92c

yyyaaammmlll......

Fix incorrect option names.

a91d6bf

trying still

760b280

reverting some Revert "reverting some" This reverts commit 13242b0. Finally?

Try to improve template layout.

e6b9c47

Don't let clang-format mangle enums into a single line.

00ab6be

turn off clang format in one spot

cb97de2

Turn off clang format in small part of Hall term.

d1c5304

one more space Y and Z clang format bypass in Hall term too.

ursg force-pushed the precommitTest branch from 5adf619 to d1c5304 Compare October 29, 2025 13:06

pre-commit-ci-lite bot and others added 5 commits October 29, 2025 13:08

[pre-commit.ci lite] apply automatic fixes

7aebeb6

Only pre-commit on pull request.

b519093

[pre-commit.ci lite] apply automatic fixes

7c00be9

align one pragma manually

aab88c8

[pre-commit.ci lite] apply automatic fixes

91e8e68

ykempf and others added 10 commits October 29, 2025 15:51

Hashes in comments? Pliiis. 😭

89f839f

[pre-commit.ci lite] apply automatic fixes

22fab03

hash in comment

44a154f

[pre-commit.ci lite] apply automatic fixes

054d005

Hash in comments

38abc5b

[pre-commit.ci lite] apply automatic fixes

447ee8c

more hashes in comment

42eb8f6

[pre-commit.ci lite] apply automatic fixes

d9e31d7

more hashes in comments pre #

7b9d72c

Another dummy commit to make extra sure...

5e7bd9b

		void setDipoleField(const FieldFunction& dipole) { dipoleField = dipole; };
		void setConstantBackgroundField(const std::array<Real, 3> B) { BGB = B; }

Conversation

ykempf commented Sep 24, 2025

Uh oh!

ursg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkotipal Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ursg commented Sep 25, 2025

Uh oh!

ykempf commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ursg commented Oct 29, 2025

Uh oh!

ursg commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lkotipal Sep 25, 2025 •

edited

Loading