Resolve compiler warnings in new measurement framework

XMLWordPrintable

Details

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

Description

When building meas_base, or any other measurement plugins which follow the same interface, with clang, I see a bunch of warnings along the lines of:

 In file included from src/ApertureFlux.cc:34: include/lsst/meas/base/ApertureFlux.h:197:18: warning: 'lsst::meas::base::ApertureFluxAlgorithm::measure' hides overloaded virtual function  [-Woverloaded-virtual]  virtual void measure(  ^ include/lsst/meas/base/Algorithm.h:183:18: note: hidden overloaded virtual function 'lsst::meas::base::SimpleAlgorithm::measure' declared here:  different number of parameters (4 vs 2)  virtual void measure(

This is an artefact of a workaround for SWIG issues; the warnings aren't indicative of a fundamental problem, but if we can avoid them we should.

While we're at it, we should also fix:

 include/lsst/meas/base/ApertureFlux.h:233:1: warning: 'ApertureFluxResult' defined as a struct here but previously declared as a class  [-Wmismatched-tags] struct ApertureFluxResult : public FluxResult { ^ include/lsst/meas/base/ApertureFlux.h:65:1: note: did you mean struct here? class ApertureFluxResult; ^~~~~ struct

Activity

No builds found.
John Swinbank created issue -
Field Original Value New Value
Epic Link DM-2044 [ 16177 ]
 Watchers John Swinbank [ John Swinbank ] Jim Bosch, John Swinbank [ Jim Bosch, John Swinbank ]
 Watchers Jim Bosch, John Swinbank [ Jim Bosch, John Swinbank ] Jim Bosch, John Swinbank, Lauren MacArthur, Perry Gee [ Jim Bosch, John Swinbank, Lauren MacArthur, Perry Gee ]
Hide
John Swinbank added a comment -

Jim Bosch – Perry and Lauren raised this issue. My (simple, ugly) workaround was simply to add pragmas like this everywhere the warnings are occurring:

 + #pragma clang diagnostic push + #pragma clang diagnostic ignored "-Woverloaded-virtual"  virtual void measure(  afw::table::SourceRecord & record,  afw::image::Exposure const & exposure  ) const = 0; + #pragma clang diagnostic pop

Before just ploughing ahead with that, I wanted to check that you don't have a more elegant suggestion.

Show
John Swinbank added a comment - Jim Bosch – Perry and Lauren raised this issue. My (simple, ugly) workaround was simply to add pragmas like this everywhere the warnings are occurring: + #pragma clang diagnostic push + #pragma clang diagnostic ignored "-Woverloaded-virtual" virtual void measure( afw::table::SourceRecord & record, afw::image::Exposure< float > const & exposure ) const = 0; + #pragma clang diagnostic pop Before just ploughing ahead with that, I wanted to check that you don't have a more elegant suggestion.
Hide
Jim Bosch added a comment -

A better fix would be to add using SimpleAlgorithm::measure; to the declaration for ApertureFluxAlgorithm, and any other class that exhibits these warnings.

And I of course agree we should make the struct/class usage consistent; that's always something I fail to notice, since I'm essentially always compiling with gcc and it never complains about it.

Show
Jim Bosch added a comment - A better fix would be to add using SimpleAlgorithm::measure; to the declaration for ApertureFluxAlgorithm , and any other class that exhibits these warnings. And I of course agree we should make the struct/class usage consistent; that's always something I fail to notice, since I'm essentially always compiling with gcc and it never complains about it.
Hide
John Swinbank added a comment -

A better fix would be to add using SimpleAlgorithm::measure; to the declaration for ApertureFluxAlgorithm, and any other class that exhibits these warnings.

That's clearly the right thing to do, but won't it confuse SWIG? I've not tried it, but I thought that was the point being made by the design notes.

Show
John Swinbank added a comment - A better fix would be to add using SimpleAlgorithm::measure; to the declaration for ApertureFluxAlgorithm, and any other class that exhibits these warnings. That's clearly the right thing to do, but won't it confuse SWIG? I've not tried it, but I thought that was the point being made by the design notes.
Hide
John Swinbank added a comment -

Having actually looked at the code, I think adding a using declaration is fine so long as it's also private. Given that, it's clearly better than faffing with the compiler warnings.

Show
John Swinbank added a comment - Having actually looked at the code, I think adding a using declaration is fine so long as it's also private. Given that, it's clearly better than faffing with the compiler warnings.
Hide
Jim Bosch added a comment -

I think it'd be fine public as well. I think Swig was only confused to the extent that it simply ignored the using declaration, not that it caused problems. Of course, if you've found a case where it does cause problems, just ignore me.

Show
Jim Bosch added a comment - I think it'd be fine public as well. I think Swig was only confused to the extent that it simply ignored the using declaration, not that it caused problems. Of course, if you've found a case where it does cause problems, just ignore me.
Hide
John Swinbank added a comment -

I've not looked hard, but just whacking in a public using breaks:

 python/lsst/meas/base/baseLib_wrap.cc:21816:59: error: 'measure' is a private member of  'lsst::meas::base::PsfFluxAlgorithm'  ((lsst::meas::base::PsfFluxAlgorithm const *)arg1)->measure(*arg2,(lsst::afw::image::...  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ include/lsst/meas/base/PsfFlux.h:91:18: note: declared private here  virtual void measure(

But a private version quiets the errors and doesn't seem to cause any problems, so that's an easy solution.

Show
John Swinbank added a comment - I've not looked hard, but just whacking in a public using breaks: python/lsst/meas/base/baseLib_wrap.cc:21816:59: error: 'measure' is a private member of 'lsst::meas::base::PsfFluxAlgorithm' ((lsst::meas::base::PsfFluxAlgorithm const *)arg1)->measure(*arg2,(lsst::afw::image::... ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ include/lsst/meas/base/PsfFlux.h:91:18: note: declared private here virtual void measure( But a private version quiets the errors and doesn't seem to cause any problems, so that's an easy solution.
Hide
Jim Bosch added a comment -

Hmm. I'm starting to think we'd be better off with an entirely different solution: making all of the measure methods and using declarations public, but putting them inside #ifndef SWIG blocks.

I'd like to give that a try, but feel free to just reassign to me if you need to be working on other things this weekend.

Show
Jim Bosch added a comment - Hmm. I'm starting to think we'd be better off with an entirely different solution: making all of the measure methods and using declarations public, but putting them inside #ifndef SWIG blocks. I'd like to give that a try, but feel free to just reassign to me if you need to be working on other things this weekend.
Hide
John Swinbank added a comment -

Yes, I like that solution better. I tested it on meas_base and it seems to work fine. Since there's no real urgency here, I'll hold off on finishing up this ticket until DM-982 (update photometryKron to new framework) & maybe DM-981 (ditto for shapeHSM, although that looks less imminent) have landed so I can make the same changes there on the same ticket.

Show
John Swinbank added a comment - Yes, I like that solution better. I tested it on meas_base and it seems to work fine. Since there's no real urgency here, I'll hold off on finishing up this ticket until DM-982 (update photometryKron to new framework) & maybe DM-981 (ditto for shapeHSM, although that looks less imminent) have landed so I can make the same changes there on the same ticket.
 Story Points 1
 Status To Do [ 10001 ] In Progress [ 3 ]
Hide
John Swinbank added a comment -

...also DM-980.

Show
John Swinbank added a comment - ...also DM-980 .
 Link This issue has to be done after DM-980 [ DM-980 ]
 Link This issue has to be done after DM-982 [ DM-982 ]
 Link This issue has to be done after DM-981 [ DM-981 ]
Hide
John Swinbank added a comment -

It'd be nice to get this landed in W15, but it's in the queue behind DM-980/1/2 before it can be merged.

However... the changes associated with those should basically be carbon copies of the changes I've already made in tickets/DM-2131 on meas_base; my suggestion is that if you're happy with the changes here, I can do the same thing on the other repositories without further review (assuming that all the tests pass, etc). Is that fair?

Show
John Swinbank added a comment - It'd be nice to get this landed in W15, but it's in the queue behind DM-980 /1/2 before it can be merged. However... the changes associated with those should basically be carbon copies of the changes I've already made in tickets/ DM-2131 on meas_base ; my suggestion is that if you're happy with the changes here, I can do the same thing on the other repositories without further review (assuming that all the tests pass, etc). Is that fair?
 Reviewers Lauren MacArthur [ lauren ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
Lauren MacArthur added a comment -

I am indeed happy with all of the above. Those particular warnings are now gone and all tests passed, so feel free to make and merge the other related changes when you can without further review.

Show
Lauren MacArthur added a comment - I am indeed happy with all of the above. Those particular warnings are now gone and all tests passed, so feel free to make and merge the other related changes when you can without further review.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
John Swinbank added a comment -

Jim – following our discussion, I tweaked meas_base to disambiguate measure and measureForced. Changes on tickets/DM-2131 in meas_base. Passes the tests there, and also resolves my issues in ip_diffim.

I realise you are snowed under with end-of-cycle reviews, but could you give it a quick check to make sure it's what you had in mind?

Also, since this is a change to the previously-agreed interface, any thoughts on whether it needs RFC-ing? My feeling is no.

Show
John Swinbank added a comment - Jim – following our discussion, I tweaked meas_base to disambiguate measure and measureForced . Changes on tickets/DM-2131 in meas_base . Passes the tests there, and also resolves my issues in ip_diffim . I realise you are snowed under with end-of-cycle reviews, but could you give it a quick check to make sure it's what you had in mind? Also, since this is a change to the previously-agreed interface, any thoughts on whether it needs RFC-ing? My feeling is no.
 Reviewers Lauren MacArthur [ lauren ] Jim Bosch [ jbosch ] Status Reviewed [ 10101 ] In Review [ 10004 ]
Hide
Jim Bosch added a comment -

Looks good; sorry it took me so long to take a look at this. I agree that this shouldn't require an RFC.

Show
Jim Bosch added a comment - Looks good; sorry it took me so long to take a look at this. I agree that this shouldn't require an RFC.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
John Swinbank added a comment -

Thanks! ("So long" being two weekend days? Hmm...!)

Show
John Swinbank added a comment - Thanks! ("So long" being two weekend days? Hmm...!)
Hide
John Swinbank added a comment -

I'd like to land this now, but it turns out I need to install GalSim before I can test on shapeHSM, and I doubt that's going to happen before I have to bump this to S15.

Show
John Swinbank added a comment - I'd like to land this now, but it turns out I need to install GalSim before I can test on shapeHSM , and I doubt that's going to happen before I have to bump this to S15.
 Epic Link DM-2044 [ 16177 ] DM-1910 [ 15942 ]
 Sprint Science Pipelines DM-W15-5 [ 129 ] Science Pipelines DM-W15-5, Science Pipelines DM-S15-1 [ 129, 140 ]
 Rank Ranked higher
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Component/s meas_base [ 10750 ]

People

Assignee:
John Swinbank
Reporter:
John Swinbank
Reviewers:
Jim Bosch
Watchers:
Jim Bosch, John Swinbank, Lauren MacArthur, Perry Gee