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

calling extend with a SchemaMapper should support positional arguments

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • afw
    • 1
    • Science Pipelines DM-W15-3
    • Data Release Production

    Description

      Calling catalog.extend(other, mapper) isn't equivalent to catalog.extend(other, mapper=mapper) because the second argument is the boolean deep. When a SchemaMapper is passed as the second argument, we should recognize it for what it is.

      Attachments

        Issue Links

          Activity

            jbosch Jim Bosch added a comment -

            Simon, tiny review for you. All changes on tickets/DM-1514 of afw:

            $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1514
            commit ada87983d2faf8732ff656942dc5912d8a9007cd
            Author: Jim Bosch <jbosch@astro.princeton.edu>
            Date:   Wed Dec 10 22:17:57 2014 -0500
             
                Allow SchemaMapper as positional argument to Catalog.extend
             
             python/lsst/afw/table/Catalog.i |    2 ++
             tests/testSimpleTable.py        |    2 +-
             2 files changed, 3 insertions(+), 1 deletion(-)

            jbosch Jim Bosch added a comment - Simon, tiny review for you. All changes on tickets/ DM-1514 of afw: $ git --no-pager log --stat --reverse LSST/master..tickets/DM-1514 commit ada87983d2faf8732ff656942dc5912d8a9007cd Author: Jim Bosch <jbosch@astro.princeton.edu> Date: Wed Dec 10 22:17:57 2014 -0500   Allow SchemaMapper as positional argument to Catalog.extend   python/lsst/afw/table/Catalog.i | 2 ++ tests/testSimpleTable.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)

            I'm sorry I missed this. I'll get to it right away.

            krughoff Simon Krughoff (Inactive) added a comment - I'm sorry I missed this. I'll get to it right away.

            I commented on the PR. The gist is that I think this is necessary because the arguments are in the wrong order to begin with. If it's too big a change, I'm happy for you to not do it.

            krughoff Simon Krughoff (Inactive) added a comment - I commented on the PR. The gist is that I think this is necessary because the arguments are in the wrong order to begin with. If it's too big a change, I'm happy for you to not do it.
            jbosch Jim Bosch added a comment -

            I do like your fix better than mine, and I've gone with that (after verifying that it does indeed pass the new test). Thanks!

            jbosch Jim Bosch added a comment - I do like your fix better than mine, and I've gone with that (after verifying that it does indeed pass the new test). Thanks!
            jbosch Jim Bosch added a comment -

            Found a corner case that neither of our solutions solved, so I did end up having to add an isinstance check to your version as well, but it's now rarely invoked.

            jbosch Jim Bosch added a comment - Found a corner case that neither of our solutions solved, so I did end up having to add an isinstance check to your version as well, but it's now rarely invoked.
            jbosch Jim Bosch added a comment -

            So, the new solution doesn't work the changes made on DM-1710, but the original solution does, so I'm just going to revert to that after all.

            jbosch Jim Bosch added a comment - So, the new solution doesn't work the changes made on DM-1710 , but the original solution does, so I'm just going to revert to that after all.

            People

              jbosch Jim Bosch
              jbosch Jim Bosch
              Simon Krughoff (Inactive)
              Jim Bosch, Simon Krughoff (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.