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

Clean up and document PSFEx

    XMLWordPrintable

    Details

    • Type: Story
    • Status: To Do
    • Resolution: Unresolved
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Team:
      Data Release Production

      Description

      Technical Debt amassed by porting HSC code to LSST necessitates that PSFEx and meas_extensions_psfex be cleaned up a bit. This will involve things such as converting variable names to camel case everywhere and writing markup compatible document strings. Additionally the README of the file needs to be updated.

        Attachments

          Issue Links

            Activity

            Hide
            nlust Nate Lust added a comment -

            Issues to be addressed, comments inherited from Russell's review of DM-2961:

            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]

            Delete or address:

            • _dataType = _dataTypes[t]
            • with pyfits.open(cat) as pf

            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.

            Show
            nlust Nate Lust added a comment - Issues to be addressed, comments inherited from Russell's review of DM-2961 : 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] Delete or address: _dataType = _dataTypes [t] with pyfits.open(cat) as pf 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.
            Hide
            nlust Nate Lust added a comment -

            It has been brought up that in the SConstruct build file for PSFex the references to PSFEX_DIR can and should be replaced with the scons Dir('#') convenience function. This is not high priority, as functionally it will not change, but may make debugging and readability easier, and help some edge use cases.

            Show
            nlust Nate Lust added a comment - It has been brought up that in the SConstruct build file for PSFex the references to PSFEX_DIR can and should be replaced with the scons Dir('#') convenience function. This is not high priority, as functionally it will not change, but may make debugging and readability easier, and help some edge use cases.
            Hide
            lskelvin Lee Kelvin added a comment -

            Nate Lust, we're looking over old tickets that haven't been addressed in a while to see if they can be marked as won't fix. Does this ticket still need to be actioned, or have these issues been addressed since they were initially reported?

            Show
            lskelvin Lee Kelvin added a comment - Nate Lust , we're looking over old tickets that haven't been addressed in a while to see if they can be marked as won't fix. Does this ticket still need to be actioned, or have these issues been addressed since they were initially reported?

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              nlust Nate Lust
              Watchers:
              Lee Kelvin, Nate Lust
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated: