# convert afw::table unit tests to version 1

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
2
• Sprint:
Science Pipelines DM-W15-5, Science Pipelines DM-S15-1
• Team:
Data Release Production

#### Description

Most afw::table unit tests explicitly set version 0. We should change these to test the new behaviors, not the deprecated ones.

#### Activity

Jim Bosch created issue -
Field Original Value New Value
Epic Link DM-244 [ 11506 ]
 Rank Ranked higher
 Epic Link DM-244 [ 11506 ] DM-1099 [ 13906 ]
 Labels Measurement AppsPlanning Measurement
 Team Measurement Framework [ 10205 ] Princeton [ 10301 ] Assignee Jim Bosch [ jbosch ] Perry Gee [ pgee ]
 Labels AppsPlanning Measurement SciencePipelines
 Sprint Science Pipelines DM-S14-2 [ 105 ]
 Rank Ranked lower
 Rank Ranked higher
 Sprint Science Pipelines DM-S14-2 [ 105 ]
 Rank Ranked higher
 Sprint Science Pipelines DM-W15-5 [ 129 ]
 Status To Do [ 10001 ] In Progress [ 3 ]
Hide
Perry Gee added a comment -

I would like clarification on the scope of this change. Note that the routine schema.getNames() is still coded assuming that the common separator in field names is the ".". Is it our intention to replace this with "_", or is this routine only intended for use with version 0 tables?

Show
Perry Gee added a comment - I would like clarification on the scope of this change. Note that the routine schema.getNames() is still coded assuming that the common separator in field names is the ".". Is it our intention to replace this with "_", or is this routine only intended for use with version 0 tables?
 Epic Link DM-1099 [ 13906 ] DM-1769 [ 15588 ]
 Sprint Science Pipelines DM-W15-5 [ 129 ] Science Pipelines DM-W15-5, Science Pipelines DM-S15-1 [ 129, 140 ]
 Rank Ranked higher
Hide
Jim Bosch added a comment -

I didn't realize that getNames() pays any attention to the field separators. If it does, that will also need to be fixed to pay attention to the version.

Show
Jim Bosch added a comment - I didn't realize that getNames() pays any attention to the field separators. If it does, that will also need to be fixed to pay attention to the version.
 Link This issue blocks DM-1766 [ DM-1766 ]
Hide
Perry Gee added a comment -

This is a fairly simple change to convert AFW unit tests to work with version 1 tables. Version 1 table use "_" to separate the components of field names. They also use all scalar type instead of compound types, like Point , Quadrupole ant Matrix.

Note that some of the unit tests actually test version 0 functionality, and have not been changed.

Lauren, you can pass this to John if you don't have afw table background, but I think you probably have the right experience.
--------------------------------------------------------
afw and meas_base were modified.

pgeepc2:/sandbox2/pgee/mylsst9/Linux64/afw> git diff --stat master
src/table/Schema.cc | 20 +++++++---
tests/sourceMatch.py | 7 ++--
tests/testFunctorKeys.py | 20 ++++------
tests/testSchema.py | 33 ++++++++--------
tests/testSourceTable.py | 97 +++++++++++++++++++++++++---------------------
tests/ticket2707.py | 1 -
6 files changed, 94 insertions, 84 deletions

include/lsst/meas/base/CentroidUtilities.h | 28 ++++++++++++++++++++++
include/lsst/meas/base/ShapeUtilities.h | 36 ++++++++++++++++++++++++++++
src/CentroidUtilities.cc | 6 +++++
src/ShapeUtilities.cc | 9 +++++++
4 files changed, 79 insertions

Show
Perry Gee added a comment - This is a fairly simple change to convert AFW unit tests to work with version 1 tables. Version 1 table use "_" to separate the components of field names. They also use all scalar type instead of compound types, like Point , Quadrupole ant Matrix. Note that some of the unit tests actually test version 0 functionality, and have not been changed. Lauren, you can pass this to John if you don't have afw table background, but I think you probably have the right experience. -------------------------------------------------------- afw and meas_base were modified. pgeepc2:/sandbox2/pgee/mylsst9/Linux64/afw> git diff --stat master src/table/Schema.cc | 20 +++++++--- tests/sourceMatch.py | 7 ++-- tests/testFunctorKeys.py | 20 ++++------ tests/testSchema.py | 33 ++++++++-------- tests/testSourceTable.py | 97 +++++++++++++++++++++++++--------------------- tests/ticket2707.py | 1 - 6 files changed, 94 insertions , 84 deletions include/lsst/meas/base/CentroidUtilities.h | 28 ++++++++++++++++++++++ include/lsst/meas/base/ShapeUtilities.h | 36 ++++++++++++++++++++++++++++ src/CentroidUtilities.cc | 6 +++++ src/ShapeUtilities.cc | 9 +++++++ 4 files changed, 79 insertions
 Reviewers Lauren MacArthur [ lauren ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
Perry Gee added a comment -

These are my responses to Lauren's github pull request. I don't seem to be able to edit the request, so I am responding here.

I agree with all of the requests except for the comments about the formatting of if..then..else in two places in Schema.cc. They look OK to me. In any case, they are exactly as was formatted in the original. I would like a clarification on this. The requests are in the two getNames() methods.

Show
Perry Gee added a comment - These are my responses to Lauren's github pull request. I don't seem to be able to edit the request, so I am responding here. I agree with all of the requests except for the comments about the formatting of if..then..else in two places in Schema.cc. They look OK to me. In any case, they are exactly as was formatted in the original. I would like a clarification on this. The requests are in the two getNames() methods.
Hide
Kian-Tat Lim added a comment -

I think she was referring to lines 479-480 and 502-503 (new if/else statements conditioned on getVersion() written in two lines that should probably be expanded to five).

Show
Kian-Tat Lim added a comment - I think she was referring to lines 479-480 and 502-503 (new if/else statements conditioned on getVersion() written in two lines that should probably be expanded to five).
Hide
Lauren MacArthur added a comment -

Indeed. Thanks for clarifying K-T.

Show
Lauren MacArthur added a comment - Indeed. Thanks for clarifying K-T.
Hide
Perry Gee added a comment -

I just made all the changes suggested by Lauren, and rebased to master so that there wouldn't be any conflicts. My understanding formerly was that single line unbracketed if and else clauses were allowed (though not preferred). I find them more readable. But I decided that the line was better as a conditional assignment anyway. Meanwhile, I will review the latest standard.

Lauren, please review u/pgee/DM-1076 for merge. Since it has been rebased, you will need a new clone of both meas_algorithms and meas_base are required for the unit tests to work.

Show
Perry Gee added a comment - I just made all the changes suggested by Lauren, and rebased to master so that there wouldn't be any conflicts. My understanding formerly was that single line unbracketed if and else clauses were allowed (though not preferred). I find them more readable. But I decided that the line was better as a conditional assignment anyway. Meanwhile, I will review the latest standard. Lauren, please review u/pgee/ DM-1076 for merge. Since it has been rebased, you will need a new clone of both meas_algorithms and meas_base are required for the unit tests to work.
Hide
Jim Bosch added a comment -

To clarify on the if statement question, I think it's that one-line if statements are indeed allowed, but once you add an else clause, it's considered a multi-line block and you should use braces.

Show
Jim Bosch added a comment - To clarify on the if statement question, I think it's that one-line if statements are indeed allowed, but once you add an else clause, it's considered a multi-line block and you should use braces.
Hide
Perry Gee added a comment -

I have to say that I think that is less readable, but I will follow the standard.

Show
Perry Gee added a comment - I have to say that I think that is less readable, but I will follow the standard.
Hide
Paul Price added a comment -

It's not just a matter of being readable, but it's as important to worry about maintenance. Consider:

 if (foo) bar; else baz;

Here, it's easy to accidentally remove the second line, so that it ends up as:

 if (foo) bar;

which may dramatically alter the behaviour, and there's no sign at all that anything is out of place. When you have braces it's hard to accidentally remove a line with no trace of it, and you can't just accidentally remove any old line because it won't compile.

Show
Paul Price added a comment - It's not just a matter of being readable, but it's as important to worry about maintenance. Consider: if (foo) bar; else baz; Here, it's easy to accidentally remove the second line, so that it ends up as: if (foo) bar; which may dramatically alter the behaviour, and there's no sign at all that anything is out of place. When you have braces it's hard to accidentally remove a line with no trace of it, and you can't just accidentally remove any old line because it won't compile.
Hide
Lauren MacArthur added a comment -
Show
Hide
Perry Gee added a comment -

Thanks

Show
Perry Gee added a comment - Lauren, could you clarify the status of this review? Has your issue about formatting been addressed? Thanks
Hide
Perry Gee added a comment -

OK, I see now. This 3 part operator not specifically called out in the confluence page, so I missed it.

I fixed it, and rebased u/pgee/DM-1076

Show
Perry Gee added a comment - OK, I see now. This 3 part operator not specifically called out in the confluence page, so I missed it. I fixed it, and rebased u/pgee/ DM-1076
Hide
Jim Bosch added a comment -

Lauren has recently left on a trip, and I think she'll be more or less out of contact for a week or two.

But the code does indeed look fine now; go ahead and merge.

Show
Jim Bosch added a comment - Lauren has recently left on a trip, and I think she'll be more or less out of contact for a week or two. But the code does indeed look fine now; go ahead and merge.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
Perry Gee added a comment -

I had make a last minute change, as I was using (and improving) the meas_base Centroid and ShapeUtilities, without thinking that I couldn't actually use them in afw unit tests.

I fixed some of the problem using the Point2D and Quadrupole functor keys, but there is not really a great solution for the errors. The CentroidResult, ShapeResult and their functor keys are really needed to do comparisons of the Slot error results and the results read directly from the fields. I cobbled together a solution for the testSourceTable tests, but it was not as clean as using the ResultKeys from meas_base. They allow the user to get the error values back as a CovarianceMatrix, no matter how they are stored in the table. I think this capability should be in afw, not in meas_base, but I will let Jim figure this out.

I will not seek another review, but if anyone wants to do one, I will not merge until tomorrow night. Changes are only in afw now. u/pgee/DM-1076

Show
Perry Gee added a comment - I had make a last minute change, as I was using (and improving) the meas_base Centroid and ShapeUtilities, without thinking that I couldn't actually use them in afw unit tests. I fixed some of the problem using the Point2D and Quadrupole functor keys, but there is not really a great solution for the errors. The CentroidResult, ShapeResult and their functor keys are really needed to do comparisons of the Slot error results and the results read directly from the fields. I cobbled together a solution for the testSourceTable tests, but it was not as clean as using the ResultKeys from meas_base. They allow the user to get the error values back as a CovarianceMatrix, no matter how they are stored in the table. I think this capability should be in afw, not in meas_base, but I will let Jim figure this out. I will not seek another review, but if anyone wants to do one, I will not merge until tomorrow night. Changes are only in afw now. u/pgee/ DM-1076
Hide
Jim Bosch added a comment -

Changes look fine. I'd have thought you'd be able to use a CovarianceMatrixKey for the errors, and if you tried it, I'd be curious to hear why it didn't work. But I don't think it's worth changing here, since you already have the test code working and those particular tests may not last past DM-1764 anyway.

Show
Jim Bosch added a comment - Changes look fine. I'd have thought you'd be able to use a CovarianceMatrixKey for the errors, and if you tried it, I'd be curious to hear why it didn't work. But I don't think it's worth changing here, since you already have the test code working and those particular tests may not last past DM-1764 anyway.
Hide
Perry Gee added a comment -

I thought about using the CovarianceMatrixKey, but I was having trouble constructing one of a specific type from Python. Probably just driver error – I didn't try very hard to figure it out, particularly since you had already said that I should just do enough to save the test.

I was confused by CovarianceMatrixKey anyway. Now looking it over, I guess that the right way to construct it is to create the keys myself, and in this case, pass the constructor as an array of SigmaKeys. I kind of remember doing this in C++ when you first introduced the class. I think I was expecting it to have an addFields which was as smart as CentroidResultKey or ShapeResultKey.

Show
Perry Gee added a comment - I thought about using the CovarianceMatrixKey, but I was having trouble constructing one of a specific type from Python. Probably just driver error – I didn't try very hard to figure it out, particularly since you had already said that I should just do enough to save the test. I was confused by CovarianceMatrixKey anyway. Now looking it over, I guess that the right way to construct it is to create the keys myself, and in this case, pass the constructor as an array of SigmaKeys. I kind of remember doing this in C++ when you first introduced the class. I think I was expecting it to have an addFields which was as smart as CentroidResultKey or ShapeResultKey.
Hide
Jim Bosch added a comment -

Ah, I'd forgotten that it didn't have an addFields. Makes sense that would cause problems; I should probably add that at some point.

Show
Jim Bosch added a comment - Ah, I'd forgotten that it didn't have an addFields. Makes sense that would cause problems; I should probably add that at some point.
Hide
Perry Gee added a comment -

Yes, that was the problem with the CovarianceMatrixKey. I did not correctly call the constructor. I thought it might be a Swig problem when I couldn't construct the CovarianceMatrix3fKey.

Anyway, the upshot is that an additional ErrorKey layered on top which is smart enough to create and manage the fields as well as recognize the difference between SIGMA_ONLY and FULL_COVARIANCE might be a useful addition.

Show
Perry Gee added a comment - Yes, that was the problem with the CovarianceMatrixKey. I did not correctly call the constructor. I thought it might be a Swig problem when I couldn't construct the CovarianceMatrix3fKey. Anyway, the upshot is that an additional ErrorKey layered on top which is smart enough to create and manage the fields as well as recognize the difference between SIGMA_ONLY and FULL_COVARIANCE might be a useful addition.
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Link This issue relates to DM-1681 [ DM-1681 ]

#### People

Assignee:
Perry Gee
Reporter:
Jim Bosch
Reviewers:
Lauren MacArthur
Watchers:
Jim Bosch, Kian-Tat Lim, Lauren MacArthur, Paul Price, Perry Gee