Details

    • Type: Technical task
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      Science Pipelines DM-S15-2, Science Pipelines DM-S15-3, Science Pipelines DM-S15-4
    • Team:
      Data Release Production

      Description

      In order to support backwards-compatible FITS table reading, we need to break the current assumption that everything we need to know about how to read a Record from a FITS file is contained in the Record's Schema.

      This issue involves that refactoring, without actually adding the backwards compatibility support.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment - - edited

          John Swinbank, if you're got a bit of time for reviewing while enjoying Aspen (if not, let me know, and I'll reassign), here's some afw::table refactoring for you to enjoy. More info is available on the GitHub PR:

          https://github.com/lsst/afw/pull/26

          Note that it's a diff relative to tickets/DM-2533, not master, because I'm trying to split up the reviews of all the DM-1766 subtasks to make them more digestible.

          Show
          jbosch Jim Bosch added a comment - - edited John Swinbank , if you're got a bit of time for reviewing while enjoying Aspen (if not, let me know, and I'll reassign), here's some afw::table refactoring for you to enjoy. More info is available on the GitHub PR: https://github.com/lsst/afw/pull/26 Note that it's a diff relative to tickets/ DM-2533 , not master, because I'm trying to split up the reviews of all the DM-1766 subtasks to make them more digestible.
          Hide
          swinbank John Swinbank added a comment -

          I'm happy to look at this, but I can't promise I'll get it done this week. If you're keen to have it merged before the (nominal) end of this sprint, you might need to ask somebody else – otherwise I'll get to it within a few days.

          Show
          swinbank John Swinbank added a comment - I'm happy to look at this, but I can't promise I'll get it done this week. If you're keen to have it merged before the (nominal) end of this sprint, you might need to ask somebody else – otherwise I'll get to it within a few days.
          Hide
          jbosch Jim Bosch added a comment -

          That's fine. I've got a ton of reviews out in parallel that have to all be finished before I'll be comfortable merging any of them, with some coding yet to do on my part, so I doubt you'll be the blocker in the end.

          Show
          jbosch Jim Bosch added a comment - That's fine. I've got a ton of reviews out in parallel that have to all be finished before I'll be comfortable merging any of them, with some coding yet to do on my part, so I doubt you'll be the blocker in the end.
          Hide
          swinbank John Swinbank added a comment -

          I enjoyed reading this code & found the exercise quite informative. I have added a number of comments to the pull request, but these are in general quite minor: most of them are just suggestions for where the documentation could be made a little more helpful to the reader. I've not bothered commenting on the whitespace issue that Tim brought up, but I do think splitting that into a separate commit would be helpful.

          The only more substantive concern I have here is regarding testing, or, rather, lack thereof. The high level functionality is already covered by the existing tests, so that's fine. However, those tests do cover a lot of ground at once, and I can't help but wonder if lower level tests, directed at simpler units of code (like FitsSchemaInputMapper) wouldn't be helpful to provide some more fine-grained information when things break. That said, I realise that much of this functionality isn't exposed to Python, and that we prefer to do all our testing in Python code as far as is possible – it may be that testing at this level is impractical or not worth the effort, but I think it's worth considering.

          One area where I'm concerned that the high-level test coverage may not be adequate is in the backwards-compatibility code. For example, SourceFitsReader switches between OldSourceFootprintReader and SourceFootprintReader depending on the input file. I think the test cases rely on creating some data, persisting it, and then demonstrating that it can be read correctly: presumably that will only ever test one of the possible code paths here.

          Show
          swinbank John Swinbank added a comment - I enjoyed reading this code & found the exercise quite informative. I have added a number of comments to the pull request, but these are in general quite minor: most of them are just suggestions for where the documentation could be made a little more helpful to the reader. I've not bothered commenting on the whitespace issue that Tim brought up, but I do think splitting that into a separate commit would be helpful. The only more substantive concern I have here is regarding testing, or, rather, lack thereof. The high level functionality is already covered by the existing tests, so that's fine. However, those tests do cover a lot of ground at once, and I can't help but wonder if lower level tests, directed at simpler units of code (like FitsSchemaInputMapper ) wouldn't be helpful to provide some more fine-grained information when things break. That said, I realise that much of this functionality isn't exposed to Python, and that we prefer to do all our testing in Python code as far as is possible – it may be that testing at this level is impractical or not worth the effort, but I think it's worth considering. One area where I'm concerned that the high-level test coverage may not be adequate is in the backwards-compatibility code. For example, SourceFitsReader switches between OldSourceFootprintReader and SourceFootprintReader depending on the input file. I think the test cases rely on creating some data, persisting it, and then demonstrating that it can be read correctly: presumably that will only ever test one of the possible code paths here.
          Hide
          jbosch Jim Bosch added a comment -

          The backwards compatibility test coverage is actually dealt with later in DM-1766, on DM-2536, where I've re-enabled/added two tests that operate on git-committed files that were generated with an older version of the pipeline - I didn't do it on this particular issue simply because I really needed all of the backwards-compatibility code to be in place to test any of it.

          As for the more general question of how to test intermediate-level interfaces, that's a concern bigger than just this issue. I think the right solution is to still have some tests in C++, as I don't think it's generally good for our user-visible Python interfaces to have additional Python interfaces that are only used for testing. I'll take a look at FitsSchemaInputMapper and see if there's anything I think is worth testing directly, or alternatively, if there's anything that should just be removed since it's not used right now - I've certainly been guilty of adding useless methods in the past because I imagined they'd be useful someday in the nebulous future.

          Show
          jbosch Jim Bosch added a comment - The backwards compatibility test coverage is actually dealt with later in DM-1766 , on DM-2536 , where I've re-enabled/added two tests that operate on git-committed files that were generated with an older version of the pipeline - I didn't do it on this particular issue simply because I really needed all of the backwards-compatibility code to be in place to test any of it. As for the more general question of how to test intermediate-level interfaces, that's a concern bigger than just this issue. I think the right solution is to still have some tests in C++, as I don't think it's generally good for our user-visible Python interfaces to have additional Python interfaces that are only used for testing. I'll take a look at FitsSchemaInputMapper and see if there's anything I think is worth testing directly, or alternatively, if there's anything that should just be removed since it's not used right now - I've certainly been guilty of adding useless methods in the past because I imagined they'd be useful someday in the nebulous future.
          Hide
          jbosch Jim Bosch added a comment -

          Not yet merged, but all review comments have now been addressed on DM-1766, where the final merge-to-master prep will occur.

          Show
          jbosch Jim Bosch added a comment - Not yet merged, but all review comments have now been addressed on DM-1766 , where the final merge-to-master prep will occur.

            People

            • Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              John Swinbank
              Watchers:
              Jim Bosch, John Swinbank
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel