Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Labels:
-
Story Points:4
-
Epic Link:
-
Sprint:Measurement Sprint 1
-
Team:Data Release Production
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
- is blocked by
-
- Done
Activity
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?
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? 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.
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. 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.
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
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 
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 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.
Ah, ok. So it's not a problem right now, but it will be again in the future when we change the default version.
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?