Skip to content

ENH: Convert Heaviside step function tests to itkHeavisideStepFunctionGTest#5907

Open
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest
Open

ENH: Convert Heaviside step function tests to itkHeavisideStepFunctionGTest#5907
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest

Conversation

@hjmjohnson
Copy link
Member

Convert legacy ITK CTest driver test to GoogleTest (GTest) framework.

This is part of the ongoing effort to modernize ITK's test suite by converting no-argument CTest tests to the GoogleTest framework for improved test organization, better failure reporting, and modern C++ testing practices.

🤖 Generated with Claude Code

@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from 53427cb to 51fbfaa Compare March 8, 2026 19:26
@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Mar 8, 2026
@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from 51fbfaa to cd62c40 Compare March 8, 2026 21:15
@N-Dekker
Copy link
Contributor

N-Dekker commented Mar 8, 2026

This one produces more lines of code than the original test (lines changed: 56 additions & 45 deletions). In general, I would expect (and prefer) less lines of code, when converting an old test to GTest 🤷

@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from cd62c40 to 795d92d Compare March 9, 2026 11:36
@hjmjohnson
Copy link
Member Author

This one produces more lines of code than the original test (lines changed: 56 additions & 45 deletions). In general, I would expect (and prefer) less lines of code, when converting an old test to GTest 🤷

That is usually the case, but many of the tests only printed diagnostics to the console, and when function return values can be checked for accuracy, Claude would sometimes add explicit tests. I think it is very good to test for correctness in addition to simply exercising the code.

Copy link
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Please don't add extra checks with this pull request. Please just do Test-to-GoogleTest conversion, with this PR.

@blowekamp
Copy link
Member

Please don't add extra checks with this pull request. Please just do Test-to-GoogleTest conversion, with this PR.

I think a few small additions are fine.

Also, I believe the separation of the batch of testing into separate PRs was done to accommodate a request to make review easier. However, there is a such a large number of PRs, computer generated, that the first page of PRs is full with this one task. I need to page to get to mine and others' work 🙀

For this type of operation in the future, I'd recommend just submitting a couple items, and get the human reviews on those. Perhaps selecting the interesting cases to share and discuss. Then once a standards is established making a PR with the bulk of the changes.

@N-Dekker
Copy link
Contributor

N-Dekker commented Mar 9, 2026

For this type of operation in the future, I'd recommend just submitting a couple items, and get the human reviews on those. Perhaps selecting the interesting cases to share and discuss. Then once a standards is established making a PR with the bulk of the changes.

Thanks @blowekamp ! I just posted a related topic at our Discourse, because I believe that there is a more general challenge here, how to tame AI! Discourse: AI generated pull requests overwhelming, hard to review carefully

@N-Dekker
Copy link
Contributor

N-Dekker commented Mar 9, 2026

I think a few small additions are fine.

@blowekamp Claude made the following small additions with this PR:

  // At x=0 with epsilon=0.5: Evaluate = 0.5, EvaluateDerivative = 2/pi ≈ 0.63662
  EXPECT_NEAR(functionBase0->Evaluate(0.0), 0.5, 1e-10);
  EXPECT_NEAR(functionBase0->EvaluateDerivative(0.0), 2.0 / itk::Math::pi, 1e-4);

Are you able to review those additions carefully? Honestly it would take me quite some time! I would really have to analyze the tested function to see if those expect's are meaningful. I don't think we should blindly trust AI. It's a useful, powerful tool, but it can also make mistakes! ("Hallucinations")

@hjmjohnson
Copy link
Member Author

I think a few small additions are fine.

@blowekamp Claude made the following small additions with this PR:

  // At x=0 with epsilon=0.5: Evaluate = 0.5, EvaluateDerivative = 2/pi ≈ 0.63662
  EXPECT_NEAR(functionBase0->Evaluate(0.0), 0.5, 1e-10);
  EXPECT_NEAR(functionBase0->EvaluateDerivative(0.0), 2.0 / itk::Math::pi, 1e-4);

Are you able to review those additions carefully? Honestly it would take me quite some time! I would really have to analyze the tested function to see if those expect's are meaningful. I don't think we should blindly trust AI. It's a useful, powerful tool, but it can also make mistakes! ("Hallucinations")

The values were obtained by running the test, failing, identifying the correct values, and updating the test to insert them. I insisted that Claude-code build and run the tests to insert values. I did not go back to first principles to compute the math by hand, but I assumed that the previous results that were not tested should remain the same across platforms.

When running the test, the returned value was 0.63662 (which may or may not be 2/pi), and Claude-code replaced 0.63662 with 2.0/ itk::Math::pi as its guess of what was expected as the result, based on the code in EvaluateDerivative(0.0) and the resulting output of 0.63662.

========
I'm going to put this PR into DRAFT mode to think about what a 'good' course of action is (not the 'best' course of action, because I think there will be disagreement on what is 'best').

@hjmjohnson hjmjohnson marked this pull request as draft March 9, 2026 15:27
@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch 2 times, most recently from 2f27bc6 to 691f1f2 Compare March 12, 2026 00:56
@N-Dekker N-Dekker self-requested a review March 12, 2026 17:49
@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from 691f1f2 to 0975380 Compare March 12, 2026 20:30
@hjmjohnson hjmjohnson marked this pull request as ready for review March 12, 2026 20:31
@hjmjohnson hjmjohnson requested a review from dzenanz March 12, 2026 20:31
@hjmjohnson hjmjohnson enabled auto-merge March 12, 2026 20:37
@hjmjohnson hjmjohnson dismissed N-Dekker’s stale review March 12, 2026 20:38

Resolved all requested issues.

@N-Dekker N-Dekker disabled auto-merge March 13, 2026 10:55
@hjmjohnson hjmjohnson marked this pull request as draft March 13, 2026 13:42
@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from 0975380 to c66dd9d Compare March 13, 2026 19:50
@hjmjohnson hjmjohnson marked this pull request as ready for review March 13, 2026 19:53
@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from c66dd9d to 5ab4cbd Compare March 13, 2026 22:41
@github-actions github-actions bot added the area:Video Issues affecting the Video module label Mar 13, 2026
…nGTest

Replace itkHeavisideStepFunctionTest1, itkSinRegularizedHeavisideStepFunctionTest1,
and itkAtanRegularizedHeavisideStepFunctionTest1 legacy CTest driver tests with
GoogleTest framework.
@hjmjohnson hjmjohnson force-pushed the convert-heaviside-step-function-tests-to-itkheavisidestepfunctiongtest branch from 5ab4cbd to 7f6e587 Compare March 13, 2026 23:48
@github-actions github-actions bot removed the area:Video Issues affecting the Video module label Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants