Skip to content

Conversation

@albert-github
Copy link
Contributor

@albert-github albert-github commented Mar 2, 2025

Fix "unbalanced grouping commands" Doxygen warnings:

  • Use a brute force approach to fix the "unbalanced grouping commands" Doxygen warnings by explicitly placing the ALIASES ITKStartGrouping and ITKEndGrouping around the appropriate code blocks. This involved a multi-step, trial and error process where an initial attempt was made to address such warnings by inserting the Doxygen commands @{ and @} around the identified faulty blocks, and then any new warnings appeared as a result of those changes was
    solved in an interative, manual process.
  • Remove grouping around single functions.
  • Remove grouping from Examples: the code in the Examples folder is meant to serve as files hosting code blocks used by the ITK Software Guide and such files do not appear in the ITK Doxygen documentaion. Thus, grouping has no meaningful application within these files.

The use of the Utilities/Doxygen/itkgroup.py script to introduce the Doxygen grouping markers at the appropriate places has been abandoned as this implicitly added the commands based on the layout. Grouping is hard to enforce automatically and it is also hard to get an error-free output from this script (would nearly require a compiler).

Virtually all of the grouping in ITK happens around Get/Set method pairs.

Doxygen grouping documentation:
https://www.doxygen.nl/manual/grouping.html

This is related to issue #3654

PR Checklist

  • No API changes were made (or the changes have been approved)
  • Updated API documentation (or API not changed)

@github-actions github-actions bot added the type:Documentation Documentation improvement or change label Mar 2, 2025
@albert-github
Copy link
Contributor Author

I'm looking into the commit / compilation problems.

@albert-github
Copy link
Contributor Author

@dzenanz

I fixed the errors warnings as mentioned in the ghostflow-check-master and pre-commit in 6e8c875 but I again get a red status why (especially the ghostflow-check-master?

@dzenanz
Copy link
Member

dzenanz commented Mar 2, 2025

You get the full list of messages if you click on the link. The first part is:

Errors:

commit 8b19ffd adds bad whitespace:

Modules/Core/Common/include/itkPlatformMultiThreader.h:185: new blank line at EOF.
Modules/Core/Mesh/include/itkSphereMeshSource.h:107: new blank line at EOF.
Modules/Core/Transform/include/itkTransformBase.h:213: new blank line at EOF.
Modules/Filtering/BiasCorrection/include/itkCacheableScalarFunction.h:156: new blank line at EOF.
Modules/Filtering/BiasCorrection/include/itkCompositeValleyFunction.h:188: new blank line at EOF.
Modules/Filtering/FastMarching/include/itkFastMarchingStoppingCriterionBase.h:103: new blank line at EOF.
Modules/IO/TransformMatlab/include/itkMatlabTransformIO.h:82: new blank line at EOF.

commit 8b19ffd adds Examples/IO/XML/itkParticleSwarmOptimizerSAXReader.cxx with executable permissions, but the file does not look executable.

commit 8b19ffd adds Examples/IO/XML/itkParticleSwarmOptimizerSAXWriter.cxx with executable permissions, but the file does not look executable.

commit 8b19ffd adds Examples/RegistrationITKv4/DeformableRegistration4.cxx with executable permissions, but the file does not look executable.

commit 8b19ffd adds Examples/RegistrationITKv4/DeformableRegistration6.cxx with executable permissions, but the file does not look executable.

@dzenanz
Copy link
Member

dzenanz commented Mar 2, 2025

For pre-commit:

[INFO] This may take a few minutes...
check for added large files..............................................Passed
check python ast.........................................................Passed
check for case conflicts.................................................Passed
check illegal windows names..........................(no files to check)Skipped
check json...............................................................Passed
check for merge conflicts................................................Passed
check toml...............................................................Passed
check vcs permalinks.....................................................Passed
check xml................................................................Passed
check yaml...............................................................Passed
check that scripts with shebangs are executable..........................Passed
debug statements (python)................................................Passed
detect destroyed symlinks................................................Passed
detect private key.......................................................Passed
fix end of files.........................................................Passed
forbid new submodules................................(no files to check)Skipped
forbid submodules....................................(no files to check)Skipped
mixed line ending........................................................Passed
python tests naming..................................(no files to check)Skipped
don't commit to branch...................................................Passed
trim trailing whitespace.................................................Passed
clang-format.............................................................Failed
- hook id: clang-format
- files were modified by this hook
pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/Examples/IO/XML/itkParticleSwarmOptimizerSAXReader.cxx b/Examples/IO/XML/itkParticleSwarmOptimizerSAXReader.cxx
index 72b7ca5..95f402a 100644
--- a/Examples/IO/XML/itkParticleSwarmOptimizerSAXReader.cxx
+++ b/Examples/IO/XML/itkParticleSwarmOptimizerSAXReader.cxx
@@ -57,7 +57,7 @@ ParticleSwarmOptimizerSAXReader::StartElement(const char *  name,
            this->ContextIs("/optimizer"))
   {
     std::vector<double> * bound = nullptr;
-/**@ITKEndGrouping*/
+    /**@ITKEndGrouping*/
     const char * id = this->GetAttribute(atts, "id");
     if (id == nullptr)
     {
@@ -108,7 +108,7 @@ ParticleSwarmOptimizerSAXReader::EndElement(const char * name)
       bounds.push_back(value);
     }
     this->m_OutputObject->SetParameterBounds(bounds);
-/**@ITKEndGrouping*/
+    /**@ITKEndGrouping*/
     this->m_OutputObject->SetParametersConvergenceTolerance(
       this->m_ParametersConvergenceTolerance);
   }
@@ -178,7 +178,7 @@ ParticleSwarmOptimizerSAXReader::ReadFile()
       e.SetDescription(message.c_str());
       throw e;
     }
-/**@ITKEndGrouping*/
+    /**@ITKEndGrouping*/
     if (this->m_OutputObject == nullptr)
     {
       itkExceptionMacro("Object to be read is null!\n");

@albert-github
Copy link
Contributor Author

The problem in the ghostflow looks like a false positive as this has been fixed in 6e8c875 but the ghostflow refers to 8b19ffd

So this looks to me a small problem in ghostflow, but I might be mistaken.

image

@dzenanz
Copy link
Member

dzenanz commented Mar 2, 2025 via email

@albert-github
Copy link
Contributor Author

If you squash commits and force push, that might fix it.

I'll keep it in mind.
I never did a squash nor force push so I'm a bit reluctant to do it. I'll wait till I fixed some more problems that I see in other parts.
Is there anywhere a good and understandable guide how to squash and force push ? (git is nearly a complete black box to me)

Written on cellphone, excuse my brevity.

No problem.

@dzenanz
Copy link
Member

dzenanz commented Mar 2, 2025

Possible article to read, and/or this SO answer.

@albert-github
Copy link
Contributor Author

I added a new set of changes.

  • regarding the ghostflow problem I didn't look into it yet besides that I looked for the repository whether or not I could find any reference but in vain. I think it is a bit strange to satisfy ghostflow by means of squashing the commits as I think the problem might be in the ghosflow script / configuratio (but I might be mistaken).
    I looked on the internet and found for kwrobot-v1 (https://github.com/apps/kwrobot-v1):

    Automatically checks pull requests for developers working with Kitware.

    This GitHub App requires manual configuration by its maintainers for each installation and is not managed via any web page. It is public only to facilitate separation of ownership and installation in multiple organizations.

    So it looks a bit outside my scope.

@dzenanz dzenanz force-pushed the feature/issue_3654 branch from 222250b to 5f0b209 Compare March 3, 2025 14:28
@dzenanz
Copy link
Member

dzenanz commented Mar 3, 2025

I just squashed the commits, and applied clang-format to all files. Let's see what the CI says now.

@dzenanz dzenanz force-pushed the feature/issue_3654 branch from 5f0b209 to 630350e Compare March 3, 2025 14:43
@albert-github
Copy link
Contributor Author

@dzenanz
Thank you very much.

  • Looks much better I see that you did a second squash Looks much better.
  • I'll have a look at the other problems, looks like the macOS gave a nice error message (part of the file missing...).

@dzenanz
Copy link
Member

dzenanz commented Mar 3, 2025

There are 38 compile errors, e.g.: Modules/Core/Common/include/itkPlatformMultiThreader.h:28: error: unterminated #ifndef

Indeed, the last several lines were removed from this file.

@albert-github
Copy link
Contributor Author

@dzenanz
I fixed (at least I hope so) the compilation errors, but apparently made a small mistake (the empty line ..., have to pay more attention to those things).

  • Can you remove the line and squash again (sorry for the inconvenience).

@dzenanz dzenanz force-pushed the feature/issue_3654 branch from 5457138 to 2d884aa Compare March 3, 2025 15:41
@dzenanz
Copy link
Member

dzenanz commented Mar 3, 2025

No problem. Thank you for contributing!

@dzenanz
Copy link
Member

dzenanz commented Mar 3, 2025

Looking at ITK CDash, search for "Build Names" containing PR5262. Sorting by "Start Time" column you can get the latest build, which still has plenty of build errors.

@albert-github
Copy link
Contributor Author

@dzenanz
Very interesting references, tough I only see "condensed" log files and, to be honest, for a first overview this looks like OK but sometimes it is better to have the complete overview i.e. the complete log file.
How often are these tables refreshed, especially the one for this Pull request?

It is strange that a number of files missed some last lines (but with that many files it, unfortunately, can happen). I'll try to fix them (maybe one by one but hopefully faster.

@dzenanz
Copy link
Member

dzenanz commented Mar 3, 2025

The CDash Experimental table is usually refreshed once the CI build workers finish, and report failures both to GitHub (the list of checks in the PR) and CDash. Without compile errors, a few hours. With compile errors, sooner 😄

@albert-github
Copy link
Contributor Author

@dzenanz

Thanks. I'll check it out.
I'm amazed that doxygen didn't emit warnings like More #endif's than #if's found. which should be given by the doxygen preprocessor. Something for me to investigate.
In the mean time I already found a few more files with missing end parts.

@kwrobot-v1
Copy link

kwrobot-v1 bot commented Mar 3, 2025

Errors:

  • Failed to reserve ref 7634fa8 for the merge request: invalid git ref: 'no such commit'.
  • Failed to run the checks: mr utilities error: failed to list commits of 7634fa81b792ffd4da04da8b7070161f90d7427f for https://github.com/InsightSoftwareConsortium/ITK/pull/5262: fatal: bad object 7634fa81b792ffd4da04da8b7070161f90d7427f .

@albert-github
Copy link
Contributor Author

albert-github commented Mar 3, 2025

@dzenanz

Any idea about the error from the kwrobot at #5262 (comment)

Edit looks like the ghostflow wants now to revert a change that it beforehand would like to have...

Edit 2 Looks partly like a self healing system. but now the precommit throws a similar message ...

@dzenanz
Copy link
Member

dzenanz commented Mar 3, 2025

@bradking this looks like a bug in kwrobot-v1?

@dzenanz
Copy link
Member

dzenanz commented Mar 4, 2025

It is a small number of style errors now:

  • Modules/Core/Common/include/itkDefaultConvertPixelTraits.h:84: error: comment doesn't have \class
  • Modules/IO/MeshBase/include/itkMeshConvertPixelTraits.h:91: error: comment doesn't have \class
  • Modules/Numerics/Optimizersv4/include/itkLBFGS2Optimizerv4.h:147: error: comment doesn't have \class
  • Modules/Core/Transform/include/itkAffineTransform.h:100: error: comment doesn't have \class

@albert-github
Copy link
Contributor Author

@dzenanz
I saw them as well, and have a fix ready, will be submitted soon.

@dzenanz dzenanz force-pushed the feature/issue_3654 branch from f337250 to e056835 Compare March 4, 2025 13:10
@albert-github
Copy link
Contributor Author

@dzenanz
The remark was maybe a bit strangely formulated, but had to do with the way I had to commit in the ITKSoftwareGuide repository. I didn't look at the #5262 (comment) yet. The PR InsightSoftwareConsortium/ITKSoftwareGuide#222 I used to test the procedure for the ITKSoftwareGuide repository.

As a side note maybe we should hide the comments regarding the ITK Software Guide (don't delete them as they contain still some valuable information, at least for me).

@albert-github albert-github marked this pull request as draft March 21, 2025 09:39
@albert-github
Copy link
Contributor Author

@dzenanz

From my side I finished with the update of the PR, though I see that ITK.Pixi / Pixi-Cxx (macos-14) fails but I have no idea of the reason.

@albert-github albert-github marked this pull request as ready for review March 21, 2025 18:08
@dzenanz
Copy link
Member

dzenanz commented Mar 21, 2025

The reason was:

The following tests FAILED:
	 64 - vnl_test_hungarian_algorithm (Failed)

This test is known to fail occasionally. I have restarted the failed tests.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you Albert!

We will want to squash upon merge. Alternatively, we could squash commits and amend this PR.

@albert-github
Copy link
Contributor Author

Regarding the squashing /amending I'll leave it to you.

@dzenanz
Copy link
Member

dzenanz commented Mar 24, 2025

@thewtex the PR with updated documentation is here: InsightSoftwareConsortium/ITKSoftwareGuide#225

@jhlegarreta
Copy link
Member

Squashed into a single commit, removed superficial commits, rebased on master, added a self-contained commit message body.

@dzenanz
Copy link
Member

dzenanz commented Mar 25, 2025

Should we plan to merge this today? Or give another day or two for additional review?

Fix "unbalanced grouping commands" Doxygen warnings:
- Use a brute force approach to fix the "unbalanced grouping commands"
  Doxygen warnings by explicitly placing the `ALIASES`
 `ITKStartGrouping` and `ITKEndGrouping` around the appropriate code
  blocks. This involved a multi-step, trial and error process where an
  initial attempt was made to address such warnings by inserting the
  Doxygen commands `@{` and `@}` around the identified faulty blocks,
  and then any new warnings appeared as a result of those changes was
  solved in an interative, manual process.
- Remove grouping around single functions.
- Remove grouping from `Examples`: the code in the `Examples` folder
  is meant to serve as files hosting code blocks used by the ITK
  Software Guide and such files do not appear in the ITK Doxygen
  documentaion. Thus, grouping has no meaningful application within
  these files.

The use of the `Utilities/Doxygen/itkgroup.py` script to introduce the
Doxygen grouping markers at the appropriate places has been abandoned as
this implicitly added the commands based on the layout. Grouping is hard
to enforce automatically and it is also hard to get an error-free output
from this script (would nearly require a compiler).

Virtually all of the grouping in ITK happens around `Get`/`Set` method
pairs.

Doxygen grouping documentation:
https://www.doxygen.nl/manual/grouping.html
@jhlegarreta
Copy link
Member

I am fine giving it 1 or 2 days in case anybody wants to try locally and check that the squash did not break down things. Thanks for all the work @albert-github and to those that reviewed this.

@dzenanz dzenanz merged commit 0a95afb into InsightSoftwareConsortium:master Mar 28, 2025
16 checks passed
@dzenanz
Copy link
Member

dzenanz commented Mar 28, 2025

Albert, thanks again for your efforts on improving ITK's documentation!

@albert-github albert-github deleted the feature/issue_3654 branch March 28, 2025 17:12
@jhlegarreta
Copy link
Member

We are down to 91 warnings:
https://open.cdash.org/viewBuildError.php?type=1&buildid=10297994

Mostly non-existing group warnings.

@albert-github
Copy link
Contributor Author

Regarding the comment: #5262 (comment)

@jhlegarreta
Copy link
Member

my local build is, at least, missing the Nonunit and Testkernel directories, how ta add them?

The only relevant module in Nonunit is Review, which you can activate setting the corresponding CMake variable (Module_ITKReview I think) to ON.

TestKernel contains classes that provide support for testing purposes. These should always be present if I'm not mistaken. Otherwise, setting BUILD_TESTING to ON should make these be built.

@albert-github
Copy link
Contributor Author

@jhlegarreta
Thanks I rebuild with -DModule_ITKReview=ON -DBUILD_TESTING=ON and the Nonunit is present now though the TestKernel is still not present (might be that it is due to the fact that I only build the documentation?)

For completeness my complete CMake line:

cmake -G "NMake Makefiles" -DITK_BUILD_DOCUMENTATION=ON -DDOXYGEN_EXECUTABLE=...\bin\doxygen.exe -DITK_DOXYGEN_SERVER_BASED_SEARCH=OFF -DModule_ITKReview=ON -DBUILD_TESTING=ON ..

@jhlegarreta
Copy link
Member

the TestKernel is still not present

I do not think any extra flags need to be added for TestKernel: it should be inside the Core module.

@albert-github
Copy link
Contributor Author

@jhlegarreta

I did some tests and saw the tin the EXCLUDE_PATTERN setting there is */test* on Windows this would / will match TestKernel, so I tried the setting CASE_SENSE_NAMES set to YES but this didn't help (the default for this setting is SYSTEM which maps to NO on Windows).
Removing the */test* from the EXCLUDE_PATTERN did show the results.

I tried also where I moved TestKernel to MyTestKernel and at that moment the directory MyTestKernel was shown as well.

Looks like there are some problems:

  • setting of CASE_SENSE_NAMES (ITK side)
  • matching of the directory names (doxygen side in respect to Windows)

best is that I first look into the doxygen side problem before, eventually, resetting the CASE_SENSE_NAMES to YES.

@albert-github
Copy link
Contributor Author

FYI

For the doxygen problem I've just created a proposed pull request: doxygen/doxygen#11519

@jhlegarreta
Copy link
Member

Thanks for this effort @albert-github. So, best would be to move the conversation to the appropriate issue or eventual PR now that you seem on track wrt the issues you faced in #5262 (comment).

albert-github added a commit to albert-github/ITK that referenced this pull request Apr 1, 2025
The default setting for `CASE_SENSE_NAMES` (in doxygen) is `SYSTEM` which means that on Linux it is set to `YES` and on (e.g.) Windows to `NO` (see analysis in InsightSoftwareConsortium#5262 (comment)).
Seen the usage of the `EXCLUDE_PATTERN` `*/test*` this meant on Windows that also the directory with the name `TestKernel` was matched and thus excluded.
Setting the `CASE_SENSE_NAMES` explicitly to `YES` prevents this (on Linux it won't have any effect as here the default was sufficient).
Note: Due to a bug in doxygen (see doxygen/doxygen#11519) this would still exclude the pattern. The new setting will only have an effect, on Windows, when, the not yet released, doxygen 1.14.0 or newer will be used.
dzenanz pushed a commit that referenced this pull request Apr 2, 2025
The default setting for `CASE_SENSE_NAMES` (in doxygen) is `SYSTEM` which means that on Linux it is set to `YES` and on (e.g.) Windows to `NO` (see analysis in #5262 (comment)).
Seen the usage of the `EXCLUDE_PATTERN` `*/test*` this meant on Windows that also the directory with the name `TestKernel` was matched and thus excluded.
Setting the `CASE_SENSE_NAMES` explicitly to `YES` prevents this (on Linux it won't have any effect as here the default was sufficient).
Note: Due to a bug in doxygen (see doxygen/doxygen#11519) this would still exclude the pattern. The new setting will only have an effect, on Windows, when, the not yet released, doxygen 1.14.0 or newer will be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:Documentation Documentation improvement or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants