dkirkby / bossdata

Tools for accessing SDSS BOSS data
MIT License
1 stars 3 forks source link

Problem accessing spCFrame files via bossfetch #111

Closed belaa closed 8 years ago

belaa commented 8 years ago

Trying to download spCFrame files by typing:

bossfetch --verbose --cframe qso.dat

gives an error:

TypeError: get_exposure_name() got an unexpected keyword argument 'calibrated'

kgage commented 8 years ago

Lines 125 to 137 in bossfetch:

                for i in range(plan.num_science_exposures):
                    # Add the names of spFrame and/or spCFrame files, if they are available.
                    for name, cal in calibrated.iteritems():
                        blue_name = plan.get_exposure_name(
                            i, 'blue', row[args.fiber_name], calibrated=cal)
                        if blue_name:
                            remote_paths.add(
                                finder.get_plate_path(plate=plate, filename=blue_name))
                        red_name = plan.get_exposure_name(
                            i, 'red', row[args.fiber_name], calibrated=cal)
                        if red_name:
                            remote_paths.add(
                                finder.get_plate_path(plate=plate, filename=red_name))

Definition of get_exposure_name method for Plan class in plate.py (lines 131 to 162)

    def get_exposure_name(self, sequence_number, band, fiber, ftype='spCFrame'):
        """Get the file name of a single science exposure data product.

        Use the exposure name to locate FITS data files associated with
        individual exposures.  The supported file types are:
        :datamodel:`spCFrame <PLATE4/spCFrame>`,
        :datamodel:`spFrame <PLATE4/spFrame>`,
        :datamodel:`spFluxcalib <PLATE4/spFluxcalib>` and
        :datamodel:`spFluxcorr <PLATE4/spFluxcorr>`.
        Note that this method returns None when the requested exposure is not present
        in the plan, so the return value should always be checked.

        Args:
            sequence_number(int): Science exposure sequence number, counting from zero.
                Must be less than our num_science_exposures attribute.
            fiber(int): Fiber number to identify which spectrograph to use, which must
                be in the range 1-1000 (or 1-640 for plate < 3510).
            band(str): Must be 'blue' or 'red'.
            ftype(str): Type of exposure file whose name to return.  Must be one of
                spCFrame, spFrame, spFluxcalib, spFluxcorr.  An spCFrame is assumed
                to be uncompressed, and all other files are assumed to be compressed.

        Returns:
            str: Exposure name of the form [ftype]-[cc]-[eeeeeeee].[ext] where [cc]
                identifies the spectrograph (one of b1,r1,b2,r2) and [eeeeeeee] is the
                zero-padded exposure number. The extension [ext] is "fits" for
                spCFrame files and "fits.gz" for all other file types. Returns None if
                the name is unknown for this band and fiber combination.

        Raises:
            ValueError: one of the inputs is invalid.
        """

Bossfetch is attempting to call the method using an argument 'calibrated' when it should be specifying the 'ftype' argument. It looks like this happens if either the --frame or --cframe argument is used when calling bossfetch.

kgage commented 8 years ago

It looks like the get_exposurename method in the plate.py file was changed in the 'Add support for reading per-exposure flux calib and corr vectors '_ commit on September 22. It appears bossfetch is still attempting to access the method via the old argument options. Changing the code to the following seems like it should be an easy fix.

        if args.frame or args.cframe:
            plans = {}
            ftypes = []
            if args.frame:
                ftypes.append('spFrame')
            if args.cframe:
                ftypes.append('spCFrame')
            for row in table:
                tag = (row[args.plate_name], row[args.mjd_name])
                if tag in plans:
                    plan = plans[tag]
                else:
                    plan_remote_path = finder.get_plate_plan_path(
                        plate=row[args.plate_name], mjd=row[args.mjd_name])
                    # The next line downloads the small plan file, if necessary.
                    plan_local_path = mirror.get(plan_remote_path)
                    plan = bossdata.plate.Plan(plan_local_path)
                    plans[tag] = plan
                plate = row[args.plate_name]
                for i in range(plan.num_science_exposures):
                    # Add the names of spFrame and/or spCFrame files, if they are available.
                    for ftype in ftypes:
                        blue_name = plan.get_exposure_name(
                            i, 'blue', row[args.fiber_name], ftype=ftype)
                        if blue_name:
                            remote_paths.add(
                                finder.get_plate_path(plate=plate, filename=blue_name))
                        red_name = plan.get_exposure_name(
                            i, 'red', row[args.fiber_name], ftype=ftype)
                        if red_name:
                            remote_paths.add(
                                finder.get_plate_path(plate=plate, filename=red_name))

This would replace the 'calibrations dictionary' with the 'dtypes' list, which would not only swap the input argument for the get_exposure_name to the correct type, but it would also removes iterating over both keys and values for dictionary entries (especially since the current code never actually uses the keys)

dkirkby commented 8 years ago

@kgage - Good detective work! Please go ahead and create a branch to fix this then make a pull request to get it back into the master branch.

On Wed, Nov 4, 2015 at 6:59 PM kgage notifications@github.com wrote:

It looks like the get_exposurename method in the plate.py file was changed in the 'Add support for reading per-exposure flux calib and corr vectors '_ commit on September 22. It appears bossfetch is still attempting to access the method via the old argument options.

— Reply to this email directly or view it on GitHub https://github.com/dkirkby/bossdata/issues/111#issuecomment-153940417.