Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-47

Remove unused utils functions and change eups::productDir to getPackageDir

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None
    • Location:
      This ticket, or the Data Management chat room

      Description

      As part of DM-2635 I propose to make the following changes to the C++ API of the "utils" package:

      Remove two functions we are not using anywhere:

      • guessSvnVersion: this is clearly useless now that we use git
      • stringToAny: we aren't using it, and it has no documentation

      Rename lsst::utils::eups::productDir to lsst::utils::getPackageDir and remove the version argument. Note that the version argument has only one valid value, so it's never been useful. This would be the implementation of DM-2635.

        Attachments

          Issue Links

            Activity

            No builds found.
            rowen Russell Owen created issue -
            Hide
            rowen Russell Owen added a comment -

            Here is a relevant bit from the Data Management HipChat room 2015-04-28

            [4:23 PM] Russell Owen: I'm looking through the C++ functions provided by utils and I've found two that don't seem to be used anywhere: guessSvnVersion (no surprise) and stringToAny. The latter is not even documented, though it's easy to see what it does by reading its code. Can I remove these as part of DM-2635?

            [4:26 PM] K-T Lim: Looks like the latter was expected to be used in FITS-reading code. OK with me to remove both.
            (Should really have a quick RFC to make sure everyone knows, but since there are no references, I don't see how this could hurt, and it's easy to put back.)

            [4:27 PM] Robert Lupton: As @ktl wrote stringToAny and it doesn't show up in anything I have checked out, it's good to delete. But in general we need a way to remove APIs

            Show
            rowen Russell Owen added a comment - Here is a relevant bit from the Data Management HipChat room 2015-04-28 [4:23 PM] Russell Owen: I'm looking through the C++ functions provided by utils and I've found two that don't seem to be used anywhere: guessSvnVersion (no surprise) and stringToAny. The latter is not even documented, though it's easy to see what it does by reading its code. Can I remove these as part of DM-2635 ? [4:26 PM] K-T Lim: Looks like the latter was expected to be used in FITS-reading code. OK with me to remove both. (Should really have a quick RFC to make sure everyone knows, but since there are no references, I don't see how this could hurt, and it's easy to put back.) [4:27 PM] Robert Lupton: As @ktl wrote stringToAny and it doesn't show up in anything I have checked out, it's good to delete. But in general we need a way to remove APIs
            rowen Russell Owen made changes -
            Field Original Value New Value
            Description As part of DM-2635 I propose to make the following changes to the C++ API of the "utils" package:

            Remove two functions we are not using anywhere:
            - guessSvnVersion: this is clearly useless now that we use git
            - stringToAny: we aren't using it, and it has no documentation

            Rename lsst::utils::eups::productDir to lsst::utils::getPackageDir and remove the version argument. Note that the version argument has only one valid value, so it's never been useful.
            As part of DM-2635 I propose to make the following changes to the C++ API of the "utils" package:

            Remove two functions we are not using anywhere:
            - guessSvnVersion: this is clearly useless now that we use git
            - stringToAny: we aren't using it, and it has no documentation

            Rename lsst::utils::eups::productDir to lsst::utils::getPackageDir and remove the version argument. Note that the version argument has only one valid value, so it's never been useful. This would be the implementation of DM-2635.
            Hide
            rhl Robert Lupton added a comment -

            The fact that stringToAny is not documented means that it's easy to decide to remove it. If it had had a doc string, and thus appeared in doxygen, it'd be a harder case to make.

            Show
            rhl Robert Lupton added a comment - The fact that stringToAny is not documented means that it's easy to decide to remove it. If it had had a doc string, and thus appeared in doxygen, it'd be a harder case to make.
            Hide
            ktl Kian-Tat Lim added a comment -

            Even if it were in doxygen, as long as it's unused in our code and unlikely to be used by end-user code, I'd say it's a pretty easy case to make.

            (My point about being easy to put back was that this doesn't necessarily even rise to the level of needing an RFC. Nevertheless, +1 from me.)

            Show
            ktl Kian-Tat Lim added a comment - Even if it were in doxygen, as long as it's unused in our code and unlikely to be used by end-user code, I'd say it's a pretty easy case to make. (My point about being easy to put back was that this doesn't necessarily even rise to the level of needing an RFC. Nevertheless, +1 from me.)
            Hide
            mjuric Mario Juric added a comment -

            This may be obvious to everyone, but why is getPackageDir implemented in C++?

            Show
            mjuric Mario Juric added a comment - This may be obvious to everyone, but why is getPackageDir implemented in C++?
            Hide
            rowen Russell Owen added a comment -

            getProduct is used by some C++ tests and demos (all in the afw package, in case you want to look).

            util had separate implementations in C++ and Python, and both were rather different. The C++ version raised an exception if the product was not found, which I prefer, but it had an unimplemented argument we don't need. I'm unifying them with one SWIG-wrapped C++ function getPackageDir that just takes the package name and does raise an exception if the package is not found

            Show
            rowen Russell Owen added a comment - getProduct is used by some C++ tests and demos (all in the afw package, in case you want to look). util had separate implementations in C++ and Python, and both were rather different. The C++ version raised an exception if the product was not found, which I prefer, but it had an unimplemented argument we don't need. I'm unifying them with one SWIG-wrapped C++ function getPackageDir that just takes the package name and does raise an exception if the package is not found
            Hide
            rhl Robert Lupton added a comment -

            A better solution would be to remove the use of productDir from the examples (passing the filename on the command line?), but it probably isn't worth doing as part of this issue.

            The tests are similar:

            tests/background.cc:            afwdata_dir = lsst::utils::eups::productDir("afwdata");
            tests/convolveGPU.cc:            string dataDir = lsst::utils::eups::productDir("afwdata");
            tests/statistics.cc:            afwdata_dir = lsst::utils::eups::productDir("afwdata");
            tests/testWarpGpu.cc:            string dataDir = lsst::utils::eups::productDir("afwdata");

            and we should probably remove some (do we really need C++ tests for things we test in python?) and convert the others to accept filenames from argv[], also as another issue.

            Show
            rhl Robert Lupton added a comment - A better solution would be to remove the use of productDir from the examples (passing the filename on the command line?), but it probably isn't worth doing as part of this issue. The tests are similar: tests/background.cc: afwdata_dir = lsst::utils::eups::productDir("afwdata"); tests/convolveGPU.cc: string dataDir = lsst::utils::eups::productDir("afwdata"); tests/statistics.cc: afwdata_dir = lsst::utils::eups::productDir("afwdata"); tests/testWarpGpu.cc: string dataDir = lsst::utils::eups::productDir("afwdata"); and we should probably remove some (do we really need C++ tests for things we test in python?) and convert the others to accept filenames from argv[], also as another issue.
            Hide
            mjuric Mario Juric added a comment -

            Russell Owen, thanks, I didn't realize that. Your solution seems fine, though I think it further illustrates the need to be able to easily call into Python from the C++ layer (even as a temporary solution until we get to doing something like Robert Lupton is proposing).

            Bottom line, +1 from me!

            Show
            mjuric Mario Juric added a comment - Russell Owen , thanks, I didn't realize that. Your solution seems fine, though I think it further illustrates the need to be able to easily call into Python from the C++ layer (even as a temporary solution until we get to doing something like Robert Lupton is proposing). Bottom line, +1 from me!
            rowen Russell Owen made changes -
            Link This issue blocks DM-2635 [ DM-2635 ]
            Hide
            rowen Russell Owen added a comment -

            I filed DM-2641 to handle the issue brought up by Robert Lupton.

            Show
            rowen Russell Owen added a comment - I filed DM-2641 to handle the issue brought up by Robert Lupton.
            Hide
            rowen Russell Owen added a comment -

            The discussion seems to have converged with no serious objections.

            Show
            rowen Russell Owen added a comment - The discussion seems to have converged with no serious objections.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            Hide
            tjenness Tim Jenness added a comment -

            This is a good example of even more code being moved to the C++ layer (especially in this case because it seems that if it wasn't for these tests the code could have stayed in Python land).

            Show
            tjenness Tim Jenness added a comment - This is a good example of even more code being moved to the C++ layer (especially in this case because it seems that if it wasn't for these tests the code could have stayed in Python land).
            rowen Russell Owen made changes -
            Link This issue is triggering DM-4385 [ DM-4385 ]
            rowen Russell Owen made changes -
            Status Adopted [ 10806 ] Implemented [ 11105 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Watchers:
              Kian-Tat Lim, Mario Juric, Robert Lupton, Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins

                  No builds found.