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

Port the psfextractor external library from HSC to LSST

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_extensions_psfex
    • Labels:
      None

      Attachments

        Issue Links

          Activity

          Hide
          nlust Nate Lust added a comment - - edited

          The port now builds against the lsst stack, as passes the unit test. The installation requires the tickets/DM-2961 branch on:

          FFTW required a change to enable both single and double precisions libraries, where before we were only building double precision.

          Psfex has code introduced to replace clapack functionality with code using gsl. Additional changes were made to introduce eupspkg support and change table and cfg files as necessary.

          Meas_extensions_psfex has changees to bring it inline with the lsst code base including things such as slots. There were changes to fix a dangling pointer issue exposed by the clang compiler. Finally the packaging and distribution scripts and files were updated with appropriate dependencies and build info.

          Lsst_apps only introduces meas_extensions_psfex as an optional dependency. As discussed on hipchat this seems the best place to put in this optional dependency.

          To test clone (tickets/DM-2961 branch), build, and setup fftw, psfex, and meas_extensions_psfex in that order. FFTW and psfex must be built using the eupspkg scripts. If someone has cloned the repository, running eupspkg -e prep; eupspkg -e config; eupspkg -e build; eupspkg -e, will run each step of the build process in the current directory. If you desire a sandbox of the code run eupspkg -e fetch before the other eupspkg commands. The 'binary' installation of the build will be located in a directory called _eupspkg/binary/<package_name>/<version>, but the code will exist in the current directory if run without fetch, or _eupspkg/source/ if run with fetch. Please note if you are not running the latest master of eups, it is necessary to download build and set it up as well, as there was a bug fix in eupspkg. Meas_extensions_psfex can be built with either this method, or the normal scons utility.

          To test the installation a unit test can be found in meas_extensions_psfex/tests/ which creates test stars, finds them, determines the psf, and removes the stars. The test checks to make sure the stars were adequately removed

          Show
          nlust Nate Lust added a comment - - edited The port now builds against the lsst stack, as passes the unit test. The installation requires the tickets/ DM-2961 branch on: FFTW psfex meas_extensions_psfex lsst_apps FFTW required a change to enable both single and double precisions libraries, where before we were only building double precision. Psfex has code introduced to replace clapack functionality with code using gsl. Additional changes were made to introduce eupspkg support and change table and cfg files as necessary. Meas_extensions_psfex has changees to bring it inline with the lsst code base including things such as slots. There were changes to fix a dangling pointer issue exposed by the clang compiler. Finally the packaging and distribution scripts and files were updated with appropriate dependencies and build info. Lsst_apps only introduces meas_extensions_psfex as an optional dependency. As discussed on hipchat this seems the best place to put in this optional dependency. To test clone (tickets/ DM-2961 branch), build, and setup fftw, psfex, and meas_extensions_psfex in that order. FFTW and psfex must be built using the eupspkg scripts. If someone has cloned the repository, running eupspkg -e prep; eupspkg -e config; eupspkg -e build; eupspkg -e, will run each step of the build process in the current directory. If you desire a sandbox of the code run eupspkg -e fetch before the other eupspkg commands. The 'binary' installation of the build will be located in a directory called _eupspkg/binary/<package_name>/<version>, but the code will exist in the current directory if run without fetch, or _eupspkg/source/ if run with fetch. Please note if you are not running the latest master of eups, it is necessary to download build and set it up as well, as there was a bug fix in eupspkg. Meas_extensions_psfex can be built with either this method, or the normal scons utility. To test the installation a unit test can be found in meas_extensions_psfex/tests/ which creates test stars, finds them, determines the psf, and removes the stars. The test checks to make sure the stars were adequately removed
          Hide
          swinbank John Swinbank added a comment -

          Nate has been working on this for a while now; it's definitely "in progress".

          Show
          swinbank John Swinbank added a comment - Nate has been working on this for a while now; it's definitely "in progress".
          Hide
          nlust Nate Lust added a comment -

          This code successfully builds with the lsstsw tool, and passes all the tests. A compiled version of the code can be found on tiger at the path: /tigress/HSC/users/nate/lsstsw/build

          Show
          nlust Nate Lust added a comment - This code successfully builds with the lsstsw tool, and passes all the tests. A compiled version of the code can be found on tiger at the path: /tigress/HSC/users/nate/lsstsw/build
          Hide
          nlust Nate Lust added a comment -

          Created a ticket branch in lsst_apps to add meas_extensions_psfex as an optional dependency

          Show
          nlust Nate Lust added a comment - Created a ticket branch in lsst_apps to add meas_extensions_psfex as an optional dependency
          Hide
          nlust Nate Lust added a comment -

          The whole lsst stack including meas_extensions_psfex, fftw, psfex, and lsst_apps is built on lsst-dev and can be found at /nfs/home/nate/lsstsw/build

          Show
          nlust Nate Lust added a comment - The whole lsst stack including meas_extensions_psfex, fftw, psfex, and lsst_apps is built on lsst-dev and can be found at /nfs/home/nate/lsstsw/build
          Hide
          nlust Nate Lust added a comment -

          I timed the speed difference between using clapack and my hacked_clapack which uses gsl, and found the speed difference to be on the order of 0.01 seconds. This may even be particular implementation details which could be refined if necessary.

          Show
          nlust Nate Lust added a comment - I timed the speed difference between using clapack and my hacked_clapack which uses gsl, and found the speed difference to be on the order of 0.01 seconds. This may even be particular implementation details which could be refined if necessary.
          Hide
          nlust Nate Lust added a comment -

          Please feel free to give any critique of this issue you feel appropriate. I will not take anything personally. As this is my first major ticket, I'm sure I have a lot to learn still. Thank you for reviewing this.

          Show
          nlust Nate Lust added a comment - Please feel free to give any critique of this issue you feel appropriate. I will not take anything personally. As this is my first major ticket, I'm sure I have a lot to learn still. Thank you for reviewing this.
          Hide
          rowen Russell Owen added a comment - - edited

          Here is my review of the changes to psfex. I'll look at the other packages next.

          You seem to have added some temp files to git, I assume by accident. Please remove them from git and edit .gitignore to make sure files like this won't show up (I suggest emulating the .gitignore from another package, such as afw). Unwanted files include:

          • src/clapack.os
          • .sconf_temp/
          • .sconsign.dblite

          The name "hack_clapack" sounds as if the code clumsy and temporary. I assume it is good quality code that is a mininal implementation of enough functions to serve the underlying code. Perhaps a name such as lapack_functions would be clearer and less alarming?

          You added two functions in clapack.h, but they have no documentation. I am guessing these are standard lapack functions needed by the underlying code, but that's just a guess. I suggest adding enough documentation that folks can figure out what's going on (e.g. "implementation of the lapack function xxx"); I don't think full documentation is needed as long as folks can figure out where to look it up,but if the standard documentation is easily found and in good shape then might be worth copying it.

          A few lines at the top of clapack.h and f2c.h explaining what they are for would be helpful.

          What is src/test_gsl_v_cblas.c? It appears to be command-line executable test of some kind, rather than a function used by other code, in which case why is it in src? I suggest you put it in tests/ if it's a unit test, or examples/ if it's a demo of some kind (or bin/ in the unlikely event folks will want to run it regularly).

          Please add psfex and meas_extensions_psfex to the lsstsw package's repos.yaml file so that merging this ticket will not break lsstsw and thus buildbot and Jenkins. I suggest you ask somebody (Tim Jenness?) how that should be done; one obvious solution is to make a tickets/DM-3961 branch for lsstsw, but that may not be the preferred solution.

          Show
          rowen Russell Owen added a comment - - edited Here is my review of the changes to psfex. I'll look at the other packages next. You seem to have added some temp files to git, I assume by accident. Please remove them from git and edit .gitignore to make sure files like this won't show up (I suggest emulating the .gitignore from another package, such as afw). Unwanted files include: src/clapack.os .sconf_temp/ .sconsign.dblite The name "hack_clapack" sounds as if the code clumsy and temporary. I assume it is good quality code that is a mininal implementation of enough functions to serve the underlying code. Perhaps a name such as lapack_functions would be clearer and less alarming? You added two functions in clapack.h, but they have no documentation. I am guessing these are standard lapack functions needed by the underlying code, but that's just a guess. I suggest adding enough documentation that folks can figure out what's going on (e.g. "implementation of the lapack function xxx"); I don't think full documentation is needed as long as folks can figure out where to look it up,but if the standard documentation is easily found and in good shape then might be worth copying it. A few lines at the top of clapack.h and f2c.h explaining what they are for would be helpful. What is src/test_gsl_v_cblas.c? It appears to be command-line executable test of some kind, rather than a function used by other code, in which case why is it in src? I suggest you put it in tests/ if it's a unit test, or examples/ if it's a demo of some kind (or bin/ in the unlikely event folks will want to run it regularly). Please add psfex and meas_extensions_psfex to the lsstsw package's repos.yaml file so that merging this ticket will not break lsstsw and thus buildbot and Jenkins. I suggest you ask somebody (Tim Jenness?) how that should be done; one obvious solution is to make a tickets/ DM-3961 branch for lsstsw, but that may not be the preferred solution.
          Hide
          rowen Russell Owen added a comment - - edited

          Review of fftw changes

          Your changes look great. However, I do have one question: for adding the single-precision fftw library to the fftw package, would it suffice to list both libraries in fftw.cfg (change libs=["fftw3"] to libs=["fftw3", "fftw3f"]) instead of adding a new .cfg file?

          Actually, given this in meas_extensions_psfex's table file:

               "buildRequired": ["afw", "boost_test", "daf_base", "fftw_float",
          

          I think I don't fully understand what's going on. You are adding a new package named fftw_float? Is it necessary that it be a new package, or could fftw supply both double and float libraries?

          Show
          rowen Russell Owen added a comment - - edited Review of fftw changes Your changes look great. However, I do have one question: for adding the single-precision fftw library to the fftw package, would it suffice to list both libraries in fftw.cfg (change libs= ["fftw3"] to libs= ["fftw3", "fftw3f"] ) instead of adding a new .cfg file? Actually, given this in meas_extensions_psfex's table file: "buildRequired": ["afw", "boost_test", "daf_base", "fftw_float", I think I don't fully understand what's going on. You are adding a new package named fftw_float? Is it necessary that it be a new package, or could fftw supply both double and float libraries?
          Hide
          rowen Russell Owen added a comment -

          Review of meas_extensions_psfex changes

          bin/psfex.py:

          • Please don't use from <package> import * (except in __init__.py files) because it makes it hard to tell where symbols come from and defeats some error checking in linters (and so is disallowed in our coding standards).
          • Please add a help string to the parser (I don't personally find description="PSFEX" helpful). If this is a wrapper for an existing well-known PSFEX package then it would help to say that.

          python/.../psfex.py:

          • The various functions really could use more documentation, including more explanation of what each function does and doxygen markup that describes each parameter.
          • Some of the function names seem unclear, such as "makeitLsst". What is "it"? More descriptive names (longer, if need be) would make this code easier to understand. This is especially important for any functions intended to be used outside this module.
          • Please include _all_=(...) to specify the public functions in this module. And consider then importing this module in the __init__.py.
          • Is it necessary to have code such as that described as "# Hack: I want the WCS so I'll guess where the calexp is to be found" in library routines? This really scares me. We ought to be able to do this job without guessing. Perhaps the function is not being given the right parameters? Note that this same hack appears twice and is the only use for guessCalexp, so some refactoring would probably also help.
          • You open using "with pyfits.open(cat) as pf" but never use the resulting file object. I think you can simply eliminate that line and not open the file object.
          • The following function has no effect. If you mean _dataType to be a global then you need to "global _dataType" before you set it. Furthermore, it would make your code more robust to use "global _dataType" in any function that uses it, to avoid this kind of error and make it clear you are referring to a variable defined elsewhere. likewise for "_dataTypes". Also the two names are too similar and thus easily confused; I suggest _dataTypeDict for the second.

            def setDataType(t):
                _dataType = _dataTypes[t]
            

          • Please use "as" when catching exceptions, e.g. "except Exception as e:" instead of "except Exception, e:". This eliminates a potential source of confusion when catching multiple exceptions and looks ahead to Python 3. However, in the instance I see the exception isn't used anyway, so "except Exception:" is preferred.
          • pyflakes shows 9 instances of variables assigned to that are never used. Some of these are discussed above but most can probably just be deleted.
          • nobj = len(xm)
          • _dataType = _dataTypes[t]
          • fwhmode = np.empty(ncat)
          • with pyfits.open(cat) as pf
          • next = fields[0].getNext() # number of extensions
          • psfstep = prefs.getPsfStep()
          • ixy = tab.get("%s.xy" % shape)
          • except Exception, e:
          • We already have a function called showPsf. What makes yours different? Can you use the other one?
          • Some function names use underscore. Is there a compelling reason to do this, becaus our standard calls for camelCase. Even for borrowed code such as compute_fwhmrange it'd be nice to clean up the doc string to meet our standards and rename it to camelCase unless people are strongly expecting its old name.

          Speaking of names...I see Field.hh and prefs.hh which predate this ticket, but why the .hh instead of .h? I assume they are C++ wrappers around existing .h files, but if so it'd be nice to say so in the files.

          psfexPsfDeterminer.py

          • This line is hard to understand: "bool(psfex.Context.REMOVEHIDDEN if False else psfex.Context.KEEPHIDDEN))". I suggest that you set a boolean variable whose name is helpul before calling the function, then use that variable in the funciton call.

          prefsImpl.cc

          • You added array paramvalues, but don't actually do anything with it except use the current value as a temporary. Wouldn't it suffice to do this, instead:

                string const valueName{values->getAsString(name)};
                argval[i] = const_cast<char *>(valueName.c_str());
            

          PsfexPsf.cc

          • Minor point, but I suggest removing these commented-out lines that you added:

                    //averagePosition(schema.addField< afw::table::Point<double> >(
                    //                "averagePosition", "average position of stars used to make the PSF")),
            

                //table::Key<table::Point<double> > averagePosition;
            

          psfImpl.cc

          • Minor point, but please remove the trailing space you added at line 149: "} ".

          testPsfexPsf.py

          • Minor point, but please remove commented-out line 204: "#self.catalog = SpatialModelPsfTestCase.measure(self.exposure)"

          meas_excensions_psfex.table

          • Please list all packages directly used by this package, thus boost, fftw (or is it fftw_float?), afw, etc. I know it was already this way, but LSST does have the rule that all direct dependencies are to be listed in the table file.
          Show
          rowen Russell Owen added a comment - Review of meas_extensions_psfex changes bin/psfex.py: Please don't use from <package> import * (except in __init__.py files) because it makes it hard to tell where symbols come from and defeats some error checking in linters (and so is disallowed in our coding standards). Please add a help string to the parser (I don't personally find description="PSFEX" helpful). If this is a wrapper for an existing well-known PSFEX package then it would help to say that. python/.../psfex.py: The various functions really could use more documentation, including more explanation of what each function does and doxygen markup that describes each parameter. Some of the function names seem unclear, such as "makeitLsst". What is "it"? More descriptive names (longer, if need be) would make this code easier to understand. This is especially important for any functions intended to be used outside this module. Please include _ all _=(...) to specify the public functions in this module. And consider then importing this module in the __init__.py. Is it necessary to have code such as that described as "# Hack: I want the WCS so I'll guess where the calexp is to be found" in library routines? This really scares me. We ought to be able to do this job without guessing. Perhaps the function is not being given the right parameters? Note that this same hack appears twice and is the only use for guessCalexp, so some refactoring would probably also help. You open using "with pyfits.open(cat) as pf" but never use the resulting file object. I think you can simply eliminate that line and not open the file object. The following function has no effect. If you mean _dataType to be a global then you need to "global _dataType" before you set it. Furthermore, it would make your code more robust to use "global _dataType" in any function that uses it, to avoid this kind of error and make it clear you are referring to a variable defined elsewhere. likewise for "_dataTypes". Also the two names are too similar and thus easily confused; I suggest _dataTypeDict for the second. def setDataType(t): _dataType = _dataTypes[t] Please use "as" when catching exceptions, e.g. "except Exception as e:" instead of "except Exception, e:". This eliminates a potential source of confusion when catching multiple exceptions and looks ahead to Python 3. However, in the instance I see the exception isn't used anyway, so "except Exception:" is preferred. pyflakes shows 9 instances of variables assigned to that are never used. Some of these are discussed above but most can probably just be deleted. nobj = len(xm) _dataType = _dataTypes [t] fwhmode = np.empty(ncat) with pyfits.open(cat) as pf next = fields [0] .getNext() # number of extensions psfstep = prefs.getPsfStep() ixy = tab.get("%s.xy" % shape) except Exception, e: We already have a function called showPsf. What makes yours different? Can you use the other one? Some function names use underscore. Is there a compelling reason to do this, becaus our standard calls for camelCase. Even for borrowed code such as compute_fwhmrange it'd be nice to clean up the doc string to meet our standards and rename it to camelCase unless people are strongly expecting its old name. Speaking of names...I see Field.hh and prefs.hh which predate this ticket, but why the .hh instead of .h? I assume they are C++ wrappers around existing .h files, but if so it'd be nice to say so in the files. psfexPsfDeterminer.py This line is hard to understand: "bool(psfex.Context.REMOVEHIDDEN if False else psfex.Context.KEEPHIDDEN))". I suggest that you set a boolean variable whose name is helpul before calling the function, then use that variable in the funciton call. prefsImpl.cc You added array paramvalues, but don't actually do anything with it except use the current value as a temporary. Wouldn't it suffice to do this, instead: string const valueName{values->getAsString(name)}; argval[i] = const_cast<char *>(valueName.c_str()); PsfexPsf.cc Minor point, but I suggest removing these commented-out lines that you added: //averagePosition(schema.addField< afw::table::Point<double> >( // "averagePosition", "average position of stars used to make the PSF")), //table::Key<table::Point<double> > averagePosition; psfImpl.cc Minor point, but please remove the trailing space you added at line 149: "} ". testPsfexPsf.py Minor point, but please remove commented-out line 204: "#self.catalog = SpatialModelPsfTestCase.measure(self.exposure)" meas_excensions_psfex.table Please list all packages directly used by this package, thus boost, fftw (or is it fftw_float?), afw, etc. I know it was already this way, but LSST does have the rule that all direct dependencies are to be listed in the table file.
          Hide
          tjenness Tim Jenness added a comment -

          Russell Owen I agree that a name of hack_clapack gives the wrong impression of what this code is.

          There are also review comments on the Github PR.

          As for repos.yaml see RFC-75 (recently adopted) for details on how to add a new entry. You also need to make another package depend on this one.

          Show
          tjenness Tim Jenness added a comment - Russell Owen I agree that a name of hack_clapack gives the wrong impression of what this code is. There are also review comments on the Github PR. As for repos.yaml see RFC-75 (recently adopted) for details on how to add a new entry. You also need to make another package depend on this one.
          Hide
          nlust Nate Lust added a comment -

          I have addressed most of the comments on this page. Meas_extensions_psfex is now an optional dependency of lsst_apps, and there is a ticket branch in the repo to reflect this change. There are a few changes to bin/psfex.py and python/.../psfex.py which were not made. After discussion with others the best course forward seemed to be putting off changes such as documentation, and refactoring the code into ticket DM-3441. In the short term this will be considered technical debt inherited by the merge, and not part of the merge ticket itself. Comments which are not addressed in this issues are ported to the DM-3441 ticket.

          repos.yaml was updated before this went out for review, if you are still not seeing the changes, check that you are up to date with master.

          The comments on github have responses addressed to them as well.

          Show
          nlust Nate Lust added a comment - I have addressed most of the comments on this page. Meas_extensions_psfex is now an optional dependency of lsst_apps, and there is a ticket branch in the repo to reflect this change. There are a few changes to bin/psfex.py and python/.../psfex.py which were not made. After discussion with others the best course forward seemed to be putting off changes such as documentation, and refactoring the code into ticket DM-3441 . In the short term this will be considered technical debt inherited by the merge, and not part of the merge ticket itself. Comments which are not addressed in this issues are ported to the DM-3441 ticket. repos.yaml was updated before this went out for review, if you are still not seeing the changes, check that you are up to date with master. The comments on github have responses addressed to them as well.
          Hide
          rowen Russell Owen added a comment -

          fftw package

          Excellent update. Your solution of creating two separate directories for the two precisions is a good workaround to the problem that making single and double precision libraries of fftw requires two sequential build+install operations, and our code that handles .cfg.sh files apparently isn't set up to support that.

          psfex package

          Thank for renaming that one sub-component.

          eupspkg.cfg.sh: the new comment is much appreciated. At risk of making you wish you had not added it, please add a space after the initial # in each line. Also, I think there is a minor typo in the last sentence: ", and if it does..."? perhaps you meant ", and if it is then..."?):

              #..."It checks if the eups fftw
              #environment variable is set, and if it does the configuration is
              #set appropriately."
          

          That said, I think the first sentence provides enough explanation by itself.

          meas_extensions_psfex package

          Thank you for dependencies to the table file. One minor change: please omit swig, since it is not used by the code (unless I missed it somewhere).

          psfex.py:

          Please fix "from lsst.meas.extensions.psfex.utils import *" on this ticket instead of deferring it. It should only take a few minutes. One technique is to delete that line and see what pyflakes or flake8 linter say is missing, then import those symbols.

          prefsImpl.cc

          Thank you for the explanation for the paramvalues vector! However...could you please add a space after the comment delimiter, e.g. "// text..." instead of "//text...".

          Show
          rowen Russell Owen added a comment - fftw package Excellent update. Your solution of creating two separate directories for the two precisions is a good workaround to the problem that making single and double precision libraries of fftw requires two sequential build+install operations, and our code that handles .cfg.sh files apparently isn't set up to support that. psfex package Thank for renaming that one sub-component. eupspkg.cfg.sh: the new comment is much appreciated. At risk of making you wish you had not added it, please add a space after the initial # in each line. Also, I think there is a minor typo in the last sentence: ", and if it does..."? perhaps you meant ", and if it is then..."?): #..."It checks if the eups fftw #environment variable is set, and if it does the configuration is #set appropriately." That said, I think the first sentence provides enough explanation by itself. meas_extensions_psfex package Thank you for dependencies to the table file. One minor change: please omit swig, since it is not used by the code (unless I missed it somewhere). psfex.py: Please fix "from lsst.meas.extensions.psfex.utils import *" on this ticket instead of deferring it. It should only take a few minutes. One technique is to delete that line and see what pyflakes or flake8 linter say is missing, then import those symbols. prefsImpl.cc Thank you for the explanation for the paramvalues vector! However...could you please add a space after the comment delimiter, e.g. "// text..." instead of "//text...".
          Hide
          swinbank John Swinbank added a comment -

          Nate Lust – could you please write a brief description of new functionality you've added to the stack here as part of the release notes at https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+S15+release+notes ? Thanks!

          Show
          swinbank John Swinbank added a comment - Nate Lust – could you please write a brief description of new functionality you've added to the stack here as part of the release notes at https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+S15+release+notes ? Thanks!

            People

            Assignee:
            nlust Nate Lust
            Reporter:
            nlust Nate Lust
            Reviewers:
            Mario Juric, mjuric-admin, Russell Owen
            Watchers:
            John Swinbank, Mario Juric, mjuric-admin, Nate Lust, Russell Owen, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.