Fix output file basename inconsistencies by kbwestfall · Pull Request #2192 · pypeit/PypeIt

kbwestfall · 2026-09-02T20:59:03Z

In the construction of the basename used for output file, we were not accounting for the different extensions possible for each spectrograph. I ran into this issue because #2184 adds a setup_datacube script that assumes all the raw files have .fits extensions.

This PR accomplishes two things:

It extracts the function primarily used to construct the basename from the PypeItMetaData class into its own stand-alone function. This makes it easier to construct the basename at any place in the code.
It consolidates the treatment of the construction of the basename that respects the allowed extensions for each spectrograph.

As a by-product of these goals, I also extended the io.remove_suffix function to respect additional compression extensions, instead of just working with .gz files. And, I removed the old use of .split('.fits') to remove the file suffix. Instead of the latter, I use either io.remove_suffix or Path.with_suffix.

For the nit-gritty details, see the claude doc in the dev-suite.

kbwestfall · 2026-09-02T21:20:39Z

Dev-suite running....

rcooke-ast

Some changes request. Let's see what the dev suite outputs, but I've mentioned a possible issue concerning SOAR/Goodman. Everything else is pretty minor. Thanks!

rcooke-ast · 2026-09-03T13:23:29Z

            if args.mode == 'save':
-                folder = '{}_noisecheck'.format(file.split('.fits')[0])
-                if not os.path.exists(folder): os.makedirs(folder)
+                folder = Path(f'{Path(file).with_suffix("")}_noisecheck')


Is it a concern that we add _noisecheck for both 1D and 2D checks (see above)? Should we also add 1D and 2D?

Good question. I think the 1d script can accept either spec1d files or output files from 1D coadds. In the former, the spec1d is kept because it uses the original filename as the base. That won't be true for the latter, but its name can't get mixed up with the 2D file. For the 2D script, the output file will always be spec2d*_noisecheck.

This maintains the old behavior, so I think we should keep it unless you have a strong objection.

rcooke-ast · 2026-09-03T13:26:06Z

    telescope = telescopes.SOARTelescopePar()
    url = 'https://noirlab.edu/science/programs/ctio/instruments/goodman-high-throughput-spectrograph'
-    allowed_extensions = [".fz"]
+    allowed_extensions = [".fits", ".fits.fz"]


Might want to think about this... You can download .fits and .fz files from the SOAR archive, and the headers are either 0-indexed on 1-indexed. So, I think we will want to explicitly keep .fz here.

Good point. I had forgotten this. I'll restrict it back to .fits.fz. It needs to be the full extension, not just .fz to properly construct the basename now.

rcooke-ast · 2026-09-03T13:26:40Z

    # Test the allowed extensions for an oddball spectrograph
    spec5 = load_spectrograph('soar_goodman_red')
-    assert spec5.allowed_extensions == [".fz"], 'Found wrong extensions'
+    assert spec5.allowed_extensions == [".fits", ".fits.fz"], 'Found wrong extensions'


Likewise , probably want to revert this.

rcooke-ast · 2026-09-03T13:30:29Z

-            raise PypeItError(f'Missing FILENAME keyword in {spec2d_files[0]}.  Set the basename '
-                        'using the command-line option.')
+            raise PypeItError(
+                f'Missing FILENAME keyword in {spec2d_files[0]}.  Set the basename using the '


Hmmm... I'm not sure what the command-line option is (but then again, I've never used coadd2d). I just thought I'd flag it in case it's not obvious to the user what they need to do to get a basename from these messages.

Good catch. The pypeit_coadd_2dspec script does have a --basename command-line option that can be used to override the default. So I think this message is still valid (even if it's placement is less than ideal).

rcooke-ast · 2026-09-03T13:31:15Z

+        'fpacked_file'

    """
+    _compression_suffixes = ('.gz', '.bz2', '.fz')


Not sure if we want to include the fz suffix here. Otherwise, the SOAR Goodman might not work.

All the soar_goodman files in the dev-suite have the .fits.fz extension, so using .fz here is right for those files at least. Are files distributed that don't have the .fits part of the extension? The reason to have the .fz in the compression suffix list is to make sure *.fits.fz file strings are returned as *, not *.fits.

rcooke-ast · 2026-09-03T13:33:38Z

+    # NOTE: This determination of obstime is exactly what is done inside of
+    # construct_basename when obstime is not provided.  The only reason this is
+    # done here is because obstime is also returned by this function.
    obstime  = fitstbl.construct_obstime(frame)


Is it worthwhile uniting these here, or not worth the effort?

I'm of two minds. obstime is used by pypeit_steps.extract_det as an argument passed to pypeit_steps.refframe_correct, but every other time it is returned it is ignored. So it seems like we could consolidate, but it feels like it's more trouble than its worth for now.

kbwestfall · 2026-09-03T14:40:35Z

Tests pass

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  695 passed, 9436 warnings in 202.44s (0:03:22) ---
--- PYTEST UNIT TESTS PASSED  166 passed, 2803 warnings in 734.02s (0:12:14) ---
--- PYTEST VET TESTS PASSED  83 passed, 290779 warnings in 6566.10s (1:49:26) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 308/308 TESTS  ---
Total disk usage: 331.205 GiB
Testing Started at 2026-09-02T21:49:27.697102
Testing Completed at 2026-09-03T11:57:12.928783
Total Time: 14:07:45.231681

kbwestfall

Thanks for the quick review, @rcooke-ast ! I've responded to everything. Two things to draw your attention to:

Do you have strong feelings about adding the 1D/2D notation to the noise check? My edit maintains the old behavior and I think the 1D/2D will be included for most files because the output uses the input spec1d/2d files as the root name.
Are there SOAR/Goodman files that have the .fz extension without the preceding .fits? If so, I'll need to adjust, but otherwise things should work fine. All the dev-suite files include .fits.fz.

kbwestfall · 2026-09-03T14:45:18Z

            if args.mode == 'save':
-                folder = '{}_noisecheck'.format(file.split('.fits')[0])
-                if not os.path.exists(folder): os.makedirs(folder)
+                folder = Path(f'{Path(file).with_suffix("")}_noisecheck')


Good question. I think the 1d script can accept either spec1d files or output files from 1D coadds. In the former, the spec1d is kept because it uses the original filename as the base. That won't be true for the latter, but its name can't get mixed up with the 2D file. For the 2D script, the output file will always be spec2d*_noisecheck.

This maintains the old behavior, so I think we should keep it unless you have a strong objection.

kbwestfall · 2026-09-03T14:48:54Z

    telescope = telescopes.SOARTelescopePar()
    url = 'https://noirlab.edu/science/programs/ctio/instruments/goodman-high-throughput-spectrograph'
-    allowed_extensions = [".fz"]
+    allowed_extensions = [".fits", ".fits.fz"]


Good point. I had forgotten this. I'll restrict it back to .fits.fz. It needs to be the full extension, not just .fz to properly construct the basename now.

kbwestfall · 2026-09-03T14:49:08Z

    # Test the allowed extensions for an oddball spectrograph
    spec5 = load_spectrograph('soar_goodman_red')
-    assert spec5.allowed_extensions == [".fz"], 'Found wrong extensions'
+    assert spec5.allowed_extensions == [".fits", ".fits.fz"], 'Found wrong extensions'


kbwestfall · 2026-09-03T14:54:14Z

-            raise PypeItError(f'Missing FILENAME keyword in {spec2d_files[0]}.  Set the basename '
-                        'using the command-line option.')
+            raise PypeItError(
+                f'Missing FILENAME keyword in {spec2d_files[0]}.  Set the basename using the '


Good catch. The pypeit_coadd_2dspec script does have a --basename command-line option that can be used to override the default. So I think this message is still valid (even if it's placement is less than ideal).

kbwestfall · 2026-09-03T14:56:49Z

+        'fpacked_file'

    """
+    _compression_suffixes = ('.gz', '.bz2', '.fz')


All the soar_goodman files in the dev-suite have the .fits.fz extension, so using .fz here is right for those files at least. Are files distributed that don't have the .fits part of the extension? The reason to have the .fz in the compression suffix list is to make sure *.fits.fz file strings are returned as *, not *.fits.

kbwestfall · 2026-09-03T15:14:14Z

+    # NOTE: This determination of obstime is exactly what is done inside of
+    # construct_basename when obstime is not provided.  The only reason this is
+    # done here is because obstime is also returned by this function.
    obstime  = fitstbl.construct_obstime(frame)


I'm of two minds. obstime is used by pypeit_steps.extract_det as an argument passed to pypeit_steps.refframe_correct, but every other time it is returned it is ignored. So it seems like we could consolidate, but it feels like it's more trouble than its worth for now.

rcooke-ast · 2026-09-03T18:34:48Z

Thanks, @kbwestfall! Sounds good, and I'm fine leaving noisecheck as it is (I've never used it before, and if others are happy with it as is, then we should leave it).

The only thing to avoid with SOAR/Goodman is to not allow .fits as an extension. So, I think your latest changes address this, thanks!

rcooke-ast

Approved!

kbwestfall added 6 commits September 2, 2026 11:47

stand-alone construct_basename function

17111d1

style edits

5f1a22f

doc update

fc45bb2

follow-up work to deal with additional naming issues

8063938

docstring syntax fix

4a3260d

clean-up

19ead80

kbwestfall added the bug Bug fix. label Sep 2, 2026

kbwestfall mentioned this pull request Sep 2, 2026

Basename development doc pypeit/PypeIt-development-suite#429

Open

f-string bug ... again

440a4e8

kbwestfall requested a review from rcooke-ast September 2, 2026 21:08

rcooke-ast requested changes Sep 3, 2026

View reviewed changes

exclude .fits as a valid extension for soar_goodman

fcb8f2e

kbwestfall commented Sep 3, 2026

View reviewed changes

rcooke-ast approved these changes Sep 3, 2026

View reviewed changes

Uh oh!

Conversation

kbwestfall commented Sep 2, 2026

Uh oh!

kbwestfall commented Sep 2, 2026

Uh oh!

rcooke-ast 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!

kbwestfall Sep 3, 2026 • 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!

kbwestfall commented Sep 3, 2026

Uh oh!

kbwestfall 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!

kbwestfall Sep 3, 2026 • 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!

rcooke-ast commented Sep 3, 2026

Uh oh!

rcooke-ast left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kbwestfall Sep 3, 2026 •

edited

Loading

kbwestfall Sep 3, 2026 •

edited

Loading