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

Add slot support for meas_base-style outputs

    XMLWordPrintable

Details

    Description

      The slot mechanism in afwTable currently uses compound fields to save the 3 slot types: flux, centroid, and shape. Since the new measurement framework uses a flattened representation in the SourceTable where these types are saved as multiple scalar fields, the slot mechanism need to be altered to handle this new table type.

      1. An alternative to KeyTuple for storing the keys required by the slot
      2. Fixup get(Centroid, Flux, Shape) in SourceRecord to use correct keys.
      3. Fixup the single value getters (getX, getY, etc) to use the correct keys.
      4. Persist slot info to fits correctly, based on table version.

      Attachments

        Issue Links

          Activity

            jbosch Jim Bosch added a comment -

            I don't see the FunctorKey constructor you are talking about. I assume you mean something like Point2DKey(std::string const &)?

            Oops, I actually meant the one that takes a SubSchema (which is just a combination of a Schema and string: it's what you get when you say schema[string]).

            I still see a failure in testPsfFlux.py. Did you check out the DM-433 ticket branch? I don't see it on the master branch, which probably means that I caused it, but the problem was very odd (having to do with the bounding boxes not matching). SInce this was your test, I thought you might be able to spot it more easily.

            I didn't realize there was a branch for meas_base (this is one reason we typically paste the output of "git diff --stat" when requesting a review). Now that I'm trying that, I'm seeing all the tests fail. Are there any other branches I need to set up? In the meantime, I'll rebuild everything against the latest afw; maybe that's the problem.

            The problem with sourceMatch only appears when the default version is 1. You have to change BaseTable.cc to see it.

            Ah, ok. So it's not a problem right now, but it will be again in the future when we change the default version.

            I'm not sure how to fix this problem with Swig. In C++, the parameter signatures disambiguate the defineSlot methods. For now, I am going to rename the methods which take FunctorKey parameters.

            As I said, I don't think we need to fix it. I'd prefer to just remove those methods, and just leave the ones that take strings. Do you need them for anything?

            jbosch Jim Bosch added a comment - I don't see the FunctorKey constructor you are talking about. I assume you mean something like Point2DKey(std::string const &)? Oops, I actually meant the one that takes a SubSchema (which is just a combination of a Schema and string: it's what you get when you say schema[string] ). I still see a failure in testPsfFlux.py. Did you check out the DM-433 ticket branch? I don't see it on the master branch, which probably means that I caused it, but the problem was very odd (having to do with the bounding boxes not matching). SInce this was your test, I thought you might be able to spot it more easily. I didn't realize there was a branch for meas_base (this is one reason we typically paste the output of " git diff --stat " when requesting a review). Now that I'm trying that, I'm seeing all the tests fail. Are there any other branches I need to set up? In the meantime, I'll rebuild everything against the latest afw; maybe that's the problem. The problem with sourceMatch only appears when the default version is 1. You have to change BaseTable.cc to see it. Ah, ok. So it's not a problem right now, but it will be again in the future when we change the default version. I'm not sure how to fix this problem with Swig. In C++, the parameter signatures disambiguate the defineSlot methods. For now, I am going to rename the methods which take FunctorKey parameters. As I said, I don't think we need to fix it. I'd prefer to just remove those methods, and just leave the ones that take strings. Do you need them for anything?
            pgee Perry Gee added a comment -

            Yes, I should have included the git diff --stat. But I did say in the previous comment that meas_base also had a DM-433 change. There may also be an issue with the fact that setupTable was commented out in sfm.py. Probably my fault – it was causing problems when I tried to break my changes into pre-433 changes and post-433 changes.

            I am confused about the SubSchema defineSlot method. My assumption was that SubSchema were only for the old schema hierarchies, and that I should not make any assumptions about the organization of the components of a slot. Are you saying that you can define a SubSchema solely by name, without regard to the position of the components in the record?

            You are saying that I should remove the FunctorKeys defineSlot(FunctorKeys) methods, and do all defineSlot's by name only? l put these methods in to mirror the defineSlot(Keys) methods you had in the previous version. I thought you wanted this abstraction, but I am happy to remove it. I had the idea that we would want the FunctorKeys to replace the compound Field Keys, but perhaps that is not the case?

            pgee Perry Gee added a comment - Yes, I should have included the git diff --stat. But I did say in the previous comment that meas_base also had a DM-433 change. There may also be an issue with the fact that setupTable was commented out in sfm.py. Probably my fault – it was causing problems when I tried to break my changes into pre-433 changes and post-433 changes. I am confused about the SubSchema defineSlot method. My assumption was that SubSchema were only for the old schema hierarchies, and that I should not make any assumptions about the organization of the components of a slot. Are you saying that you can define a SubSchema solely by name, without regard to the position of the components in the record? You are saying that I should remove the FunctorKeys defineSlot(FunctorKeys) methods, and do all defineSlot's by name only? l put these methods in to mirror the defineSlot(Keys) methods you had in the previous version. I thought you wanted this abstraction, but I am happy to remove it. I had the idea that we would want the FunctorKeys to replace the compound Field Keys, but perhaps that is not the case?
            pgee Perry Gee added a comment -

            OK, I see how the SubSchema is intended to work now. The comment about a.x a.y etc. seems to be more confining than the code actually is.

            pgee Perry Gee added a comment - OK, I see how the SubSchema is intended to work now. The comment about a.x a.y etc. seems to be more confining than the code actually is.
            jbosch Jim Bosch added a comment -

            Actually, I realized after you asked about it that SubSchema probably won't work right now for what you want, because it does rely on periods, not underscores. To fix that, I think we'd need to move the version number from BaseTable down to Schema, which could open up new problems. So I think you should just keep using FunctorKeys the way you are, and I'll open a new ticket to figure out what to do with SubSchema (which I'll take). That probably will involve moving the version down to Schema, but I don't want that to get in the way of your work on meas_base.

            We do want the FunctorKeys to replace the compound Field Keys in general, but being able to set the slots from a set of unrelated keys was a bad idea to begin with, because we persist the slots via the names, and so we can't round-trip the slots through I/O in general if the non-name ones are what was called. If I was confident it wouldn't break anything downstream, I'd recommend moving the compound Field Key versions of defineSlot() as well.

            Any thoughts on why I'm seeing more tests fail in meas_base fail? I've tried rebuilding the whole stack with master everything and tickets/DM-433 for afw and meas_base, and I'm still seeing four test failures, not just the one you report. They seem to all be slot-related (e.g. calls to getShape() when the shape slot is not set). Could you make sure you've pushed all your commits? If you don't have any ideas, I'll debug deeper on my end.

            jbosch Jim Bosch added a comment - Actually, I realized after you asked about it that SubSchema probably won't work right now for what you want, because it does rely on periods, not underscores. To fix that, I think we'd need to move the version number from BaseTable down to Schema, which could open up new problems. So I think you should just keep using FunctorKeys the way you are, and I'll open a new ticket to figure out what to do with SubSchema (which I'll take). That probably will involve moving the version down to Schema, but I don't want that to get in the way of your work on meas_base. We do want the FunctorKeys to replace the compound Field Keys in general, but being able to set the slots from a set of unrelated keys was a bad idea to begin with, because we persist the slots via the names, and so we can't round-trip the slots through I/O in general if the non-name ones are what was called. If I was confident it wouldn't break anything downstream, I'd recommend moving the compound Field Key versions of defineSlot() as well. Any thoughts on why I'm seeing more tests fail in meas_base fail? I've tried rebuilding the whole stack with master everything and tickets/ DM-433 for afw and meas_base, and I'm still seeing four test failures, not just the one you report. They seem to all be slot-related (e.g. calls to getShape() when the shape slot is not set). Could you make sure you've pushed all your commits? If you don't have any ideas, I'll debug deeper on my end.
            pgee Perry Gee added a comment -

            I cut and pasted the old code for defineCentroid and defineShape back into the Source.h.m4, but left the SubSchema code in, surrounded with comments.

            I removed the code to define slots from Functor Keys. I was never actually sure how it would be used, and it is not clear that it is useful.

            I am going to check to code in and merge to master as soon as one of use figures out what happened to testPsfFlux.py

            pgee Perry Gee added a comment - I cut and pasted the old code for defineCentroid and defineShape back into the Source.h.m4, but left the SubSchema code in, surrounded with comments. I removed the code to define slots from Functor Keys. I was never actually sure how it would be used, and it is not clear that it is useful. I am going to check to code in and merge to master as soon as one of use figures out what happened to testPsfFlux.py

            People

              pgee Perry Gee
              jbosch Jim Bosch
              Jim Bosch
              Jim Bosch, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.