Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-18314: reduce namespace confusion for matchOptimisticB #116

Merged
merged 2 commits into from Mar 9, 2019

Conversation

PaulPrice
Copy link
Contributor

The 'matchOptimisticB' namespace is overloaded, which makes it
impossible to do:
import lsst.meas.astrom.matchOptimisticB.matchOptimisticBContinued
lsst.meas.astrom.matchOptimisticB.matchOptimisticBContinued.MatchOptimisticBTask
results in:
AttributeError: 'builtin_function_or_method' object has no attribute 'matchOptimisticBContinued'

The 'matchOptimisticB' module contains a function called
'matchOptimisticB' which is being imported over the top of the
module, so rename that function. Move the MatchOptimisticBTask
from the 'matchOptimisticB' module so it can be accessed
without problems. Added a test that detects the problem.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the pickle test and for fixing this problem. I just hope you can do it in a way that avoids renaming the wrapped C++ function.

@PaulPrice
Copy link
Contributor Author

@r-owen: I've changed it so that the function name is preserved. I'm not completely happy with this solution, as the function is still being imported into the lsst.meas.astrom namespace (via the matchOptimisticBTask) and clobbering the module, but it seems that we're escaping the nasty effects of that by luck. Nevertheless, a test is now in place to ensure our luck continues.

@r-owen
Copy link
Contributor

r-owen commented Mar 8, 2019

If it works feel free to merge. But consider omitting "matchOptimisticB" from __all__ in matchOptimisticBContinued.py. Then it wont be hoisted and I think we'd both be happier

The 'matchOptimisticB' namespace is overloaded, which makes it
impossible to do:
    import lsst.meas.astrom.matchOptimisticB.matchOptimisticBContinued
    lsst.meas.astrom.matchOptimisticB.matchOptimisticBContinued.MatchOptimisticBTask
results in:
    AttributeError: 'builtin_function_or_method' object has no attribute 'matchOptimisticBContinued'

The 'matchOptimisticB' module contains a function called
'matchOptimisticB' which was being imported over the top of the
module, so leave it in the module. Move the MatchOptimisticBTask
from the 'matchOptimisticB' module so it can be accessed
without problems. Added a test that detects the problem.
src/sip/CreateWcsWithSip.cc: In constructor 'lsst::meas::astrom::sip::CreateWcsWithSip<MatchT>::CreateWcsWithSip(const std::vector<T>&, const lsst::afw::geom::SkyWcs&, int, const lsst::geom::Box2I&, int)':
src/sip/CreateWcsWithSip.cc:145:46: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
     if (_bbox.isEmpty() && !_matches.empty() > 0) {
                                              ^

Looks like a '_matches.size() > 0' was only half-changed to
'!_matches.empty()'.
@PaulPrice PaulPrice merged commit 33f1e6a into master Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants