Conversation
Fix bug that caused some Polygons defined on the sky to be misinterpreted by astMask. Previously, a Polygon that was supplied such that the unnegated Region represent more than half the sky was effectively inverted by astMask (but not by astTranN). The fix was to reverse the order of the supplied vertices and then set the Invert attribute when constructing the Polygon, if the supplied Polygon represents more than half the sky.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #57 +/- ##
==========================================
+ Coverage 54.88% 57.66% +2.77%
==========================================
Files 372 371 -1
Lines 116841 116921 +80
Branches 35679 35706 +27
==========================================
+ Hits 64126 67420 +3294
+ Misses 35601 31212 -4389
- Partials 17114 18289 +1175 ☔ View full report in Codecov by Harness. |
|
@timj Not sure what else needs doing on this one. The test program created by claude now passes, as do all the other tests. Can I leave you to merge the PR and close the issue? |
|
Thank you. I will take a look and merge. Claude has found a few other bugs as well that I will try to do PRs for. |
Passes on this branch.
Previously tests were failing and nothing noticed.
…t arrays yamlchan: to be on the safe side mxdim should just be 2 here (cherry picked from commit e6c169d; the prologue History entry was adapted to drop the asdf/transform/divide line, as that change is not present on this branch.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This is technically UB since it's setting it outside the range of the object str points to, even though it's never read from and ASAN complains rightly here, even if it's unlikely to produce a runtime bug. Had to change this into a do/while loop to retain the correct logic. (cherry picked from commit eb018e2) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ctest decides pass/fail purely from the program exit code (there is no pass/fail regular expression), so a test that always returns 0 reports success even when its own checks fail. These programs ran off the end of main() with no return statement (an implicit "return 0" in C99) and so had been passing silently regardless of their results. Return astOK ? 0 : 1 instead. testthreads also had this defect but needed a functional rewrite, so it is handled in a separate commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
testregions.c was produced by automatic translation of testregions.f and carried several defects that its (previously ignored) exit status was now reporting: - Every checkX function printed its "X tests failed" message unconditionally; the Fortran "if status != OK" guard had been dropped. - checkBox, checkCircle and checkEllipse each used the wrong FITS header: a single unrelated card block had been copied into all three, where the Fortran uses a different header per function (checkEllipse needs the pole-centred WCS, otherwise the near-pole test points transform to BAD and astEllipse rejects them). - checkdump compared the dump strings with strcmp; the Fortran instead checks astOverlap()==5, which tolerates the legitimate textual variation seen after a Region dump/restore round trip. Added the overlap fallback. - Four "frm1 = astFrame(...)" statements had been absorbed into // comments and so never executed, leaving frm1 with a stale value. The astGetRegionDisc check in checkCmpRegion uses tolerances relaxed to 1e-7 (from the Fortran's 1e-9/1e-8): that disc is fitted to a subsampled boundary mesh, so it varies at the ~1e-8 level with build/optimisation settings. The Fortran test built against the autoconf AST passes the tight tolerance on the same machine, confirming this is a build difference rather than a logic error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
testthreads relied on the Starlink EMS calls errStat/errAnnul to recover the AST__LCKERR status raised in a worker thread. In the standalone CMake build those are no-op stubs (errStat just returns astOK ? 0 : 1), so the expected lock error could never be detected and the test could not pass once it returned a real exit status. Rework it so each worker watches its own thread-local status (AST keeps a separate status per thread) and reports the outcome back through the shared MyData struct, which the main thread then checks. Also adds the missing exit-status return. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
testyamlchan reads imaging_wcs.asdf, tanSipWcs.txt and lsst_wcs.txt as inputs. They exist in the source tree but were never configured into the build directory, so the test failed at run time with "No such file" (masked until the program started returning a real exit status). Add the missing configure_file calls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record the exit-status audit, the testregions.c defects fixed against the Fortran original, and the testthreads EMS rework. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I added the failing test and then realized that it wasn't failing. Turned out some C tests weren't returning bad status. Fixing that led to some tests failing because of bad Fortran to C conversion. Fixing those led to me needing the overrun fixes from #39 (@embray this will cause a rebase issue for you with the prolog -- sorry). |
|
@timj Not a problem. See also my comment here: #55 (comment) I could maybe also pull that change out to a separate PR but since that one is ready as far as I'm concerned maybe it can stay there. A few other tests have suspiciously low line coverage. A lot of that is just due to failure conditions that should be excluded from the coverage analysis. Though a few, especially testregions.c, don't seem to be running properly at all. I'm going to take a look at that. |
|
Didn't this PR turn on the regions tests that were all failing previously? |
|
You've lost me there I'm afraid. Which region tests? And I'm not sure what you mean by turning them on. |
|
I am talking about testregions.c (The port of the fortran) which was not checking status properly and so a test was failing early on (because of a porting error) and then no other tests were running with the test returning good status overall. |
|
I also only just noticed that testpolygonmask.c was added on this PR but was not added to the CMake test build. |
|
Sorry Tim, I'm not sure what's going on here. As far as I can remember all I did on this PR was to commit the original fix in polygon.c. |
|
Sorry. My fault
|
3 participants
Fix bug that caused some Polygons defined on the sky to be misinterpreted by astMask. Previously, a Polygon that was supplied such that the unnegated Region represent more than half the sky was effectively inverted by astMask (but not by astTranN). The fix was to reverse the order of the supplied vertices and then set the Invert attribute when constructing the Polygon, if the supplied Polygon represents more than half the sky.
Fixes #56