Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-20842

Move FileDescriptor to a property in Formatter

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: daf_butler
    • Labels:
      None

      Description

      Discussing DM-20763 with John Parejko leads me to think that datastore should guarantee that Formatter instances are not reused. This would allow formatter implementations to cache information with the knowledge that they don't have to worry about the cache being wrong if the file descriptor changes. To make this more obvious I'm going to investigate changing Formatter so that FileDescriptor is a mandatory constructor argument becoming a read-only property. This will make it impossible for the formatter to be used for any other file and will mean that the argument is removed from the read and write methods.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment - - edited

            You'll have to also update HyperSuprimeCamRawFormatter.readImage() and obs.base.FitsRawFormatterBase on this PR. I think you can just cherry-pick my commits from DM-20763.

            Show
            Parejkoj John Parejko added a comment - - edited You'll have to also update HyperSuprimeCamRawFormatter.readImage() and obs.base.FitsRawFormatterBase on this PR. I think you can just cherry-pick my commits from DM-20763 .
            Hide
            tjenness Tim Jenness added a comment -

            I know that I have more to do. My concern was that this is such a major change to the formatter API that I wanted you and Jim Bosch to sign off before I proceeded to make all the other packages work.

            Show
            tjenness Tim Jenness added a comment - I know that I have more to do. My concern was that this is such a major change to the formatter API that I wanted you and Jim Bosch to sign off before I proceeded to make all the other packages work.
            Hide
            tjenness Tim Jenness added a comment -

            Given that Jim Bosch has signed off on the PR in daf_butler I'll now work on getting obs_subaru etc fixed up.

            Show
            tjenness Tim Jenness added a comment - Given that Jim Bosch has signed off on the PR in daf_butler I'll now work on getting obs_subaru etc fixed up.
            Hide
            tjenness Tim Jenness added a comment -

            I've cleaned this up and I think it does look better. At Jim Bosch's request I have allowed FormatterFactory to retrieve the class as well as an instance and that has made things more explicit. I think we should probably change Instrument.getRawFormatter() to return a class rather than an instance now but I think that can be done as part of DM-20763.

            Show
            tjenness Tim Jenness added a comment - I've cleaned this up and I think it does look better. At Jim Bosch 's request I have allowed FormatterFactory to retrieve the class as well as an instance and that has made things more explicit. I think we should probably change Instrument.getRawFormatter() to return a class rather than an instance now but I think that can be done as part of DM-20763 .
            Hide
            jbosch Jim Bosch added a comment -

            Everything looks good to me.  Just a few extremely minor comments on the daf_butler PR.  Leaving "In Review" only because there are two of us.

             

            Show
            jbosch Jim Bosch added a comment - Everything looks good to me.  Just a few extremely minor comments on the daf_butler PR.  Leaving "In Review" only because there are two of us.  
            Hide
            Parejkoj John Parejko added a comment -

            A couple comments on the PRs, but this looks good. Thanks for doing it.

            Show
            Parejkoj John Parejko added a comment - A couple comments on the PRs, but this looks good. Thanks for doing it.

              People

              • Assignee:
                tjenness Tim Jenness
                Reporter:
                tjenness Tim Jenness
                Reviewers:
                Jim Bosch, John Parejko
                Watchers:
                Jim Bosch, John Parejko, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel