Skip to content

Cleanup PDL::IO::FITS tests, fix incorrect tests; fix regression in writing 0D ndarrays #540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

djerius
Copy link
Contributor

@djerius djerius commented Apr 17, 2025

t/fits.t reorganization and fixes:

  • many tests shared variables, requiring explicit re-initialization
  • some regression tests for legacy FITS header writing were incorrectly excluded if Astro::FITS::HEADER was installed
  • the test for tilde expansion in file names was never run.
  • temporary files were explicitly unlinked instead of using the automatic facilities of File::Temp

Tests are now isolated into subtests, with no shared variables. The other issues were dealt with, but the level of header checking with and without Astro::FITS::Header is still quite asymmetric.

bug fixes:

  • null hdus were always written using the legacy Perl code due to an incorrect check if Astro::FITS::Header was available.

regression fixes:

  • passing 0D ndarrays to wfits caused an internal crash, as the code
    expected they be at least 1D. This used to work.

djerius added 5 commits April 17, 2025 12:37
…ng; fix temp file race conditions; fix tilde expansion test;

- Tests are now better isolated by using subtests and not reusing
  variables between tests

- ile::Temp's automatic file unlinking is now used, rather than
  explicitly unlinking the temp files

bugfixes;

- use of unique temporary file names could fail due to race conditions.

  File::Temp creates the temporary file and returns a handle to it.
  The file is detroyed when the handle is destroyed, and the filename
  can then be reused by another process.

  The code kept the name of the temporary file, but not the handle, so
  the file was destroyed before the code could use it, and thus
  a race condition could have ensued.

- the tilde expansion tests did not actually run

  The tests should be skipped if the '~' expanded directory was not
  writeable.  The test for that was essentially

      -w '~'

  which checks for writeability of a path which is exactly the
  character '~', not the expanded path.
@coveralls
Copy link

coveralls commented Apr 17, 2025

Coverage Status

coverage: 67.509% (+0.1%) from 67.399%
when pulling d9a7413 on djerius:pdl_io_fits
into 638ca16 on PDLPorters:master.

Copy link
Member

@mohawk2 mohawk2 left a comment

Choose a reason for hiding this comment

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

Please make the changes noted. It's a bit surprising that you submitted this without (from the CI results) even running it locally.

@mohawk2
Copy link
Member

mohawk2 commented Apr 17, 2025

regression fixes:

  • passing 0D ndarrays to wfits caused an internal crash, as the code expected they be at least 1D. This used to work.

Can you confirm in which version that used to work? It will need noting in Changes (which I'll update).

@djerius
Copy link
Contributor Author

djerius commented Apr 17, 2025

Please make the changes noted. It's a bit surprising that you submitted this without (from the CI results) even running it locally.

Harsh, dude. No need for that.

djerius added 9 commits April 17, 2025 18:47
…he raw fits file

Unify with/without Astro::FITS::Header tests for writing nullhdu, as
patht/fits-noah.t takes care of running without Astro::FITS::Header.

Scanning the raw fits file for expected COMMENT strings results in horrendous
garbage output if the tests fail. Instead use rfitshdr to extract
the header and scan the comments.
@djerius
Copy link
Contributor Author

djerius commented Apr 17, 2025

regression fixes:

  • passing 0D ndarrays to wfits caused an internal crash, as the code expected they be at least 1D. This used to work.

Can you confirm in which version that used to work? It will need noting in Changes (which I'll update).

I know with certainty it worked back in 2.018. I'll try to track down the version where it changed.

@djerius djerius requested a review from mohawk2 April 17, 2025 23:53
@djerius
Copy link
Contributor Author

djerius commented Apr 18, 2025

regression fixes:

  • passing 0D ndarrays to wfits caused an internal crash, as the code expected they be at least 1D. This used to work.

Can you confirm in which version that used to work? It will need noting in Changes (which I'll update).

I know with certainty it worked back in 2.018. I'll try to track down the version where it changed.

git bisect says 899a615.

git bisect run script:

git reset --hard
perl Makefile.PL
make -j80 && perl -Mblib -MPDL -MPDL::IO::FITS=wfits -E 'wfits { a => pdl(3)}, q{/tmp/foo}'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants