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

Port meas_mosaic to log package

    XMLWordPrintable

    Details

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

      Description

      Switch from using pex_logging to log in meas_mosaic.

        Attachments

          Issue Links

            Activity

            No builds found.
            hchiang2 Hsin-Fang Chiang created issue -
            hchiang2 Hsin-Fang Chiang made changes -
            Field Original Value New Value
            Remote Link This issue links to "Page (Confluence)" [ 14400 ]
            hchiang2 Hsin-Fang Chiang made changes -
            Link This issue blocks DM-8365 [ DM-8365 ]
            swinbank John Swinbank made changes -
            Epic Link DM-8136 [ 27591 ]
            swinbank John Swinbank made changes -
            Team Data Release Production [ 10301 ]
            Hide
            swinbank John Swinbank added a comment -

            Frossie Economou requests that we tackle this within the first couple of months of S17.

            Show
            swinbank John Swinbank added a comment - Frossie Economou requests that we tackle this within the first couple of months of S17.
            swinbank John Swinbank made changes -
            Story Points 2
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            This page in the Developer Guide may be useful: https://developer.lsst.io/coding/logging.html

            Show
            hchiang2 Hsin-Fang Chiang added a comment - This page in the Developer Guide may be useful: https://developer.lsst.io/coding/logging.html
            swinbank John Swinbank made changes -
            Assignee Perry Gee [ pgee ]
            swinbank John Swinbank made changes -
            Sprint DRP S17-1 [ 303 ]
            pgee Perry Gee made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            pgee Perry Gee added a comment -

            I substituted the lsst.log.Log.getDefaultLogger() for the lsst.pex.logging.getDefaultLog(). That appeared to be the only change required.

            The two loggers are not identical in behavio r(some of the methods, such as log.debug differ). But by default they log only to the screen and are unnamed.

            I do not see any calls in this package which differ between the two packages.

            Change is in tickets/DM-8358

            Show
            pgee Perry Gee added a comment - I substituted the lsst.log.Log.getDefaultLogger() for the lsst.pex.logging.getDefaultLog(). That appeared to be the only change required. The two loggers are not identical in behavio r(some of the methods, such as log.debug differ). But by default they log only to the screen and are unnamed. I do not see any calls in this package which differ between the two packages. Change is in tickets/ DM-8358
            pgee Perry Gee made changes -
            Reviewers Lauren MacArthur [ lauren ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            pgee Perry Gee added a comment -

            This is the same change as the ci_hsc one. I would have preferred that lauren run it, since she can actually run meas_mosaic. But you can choose to review it without executing it, I believe.

            Show
            pgee Perry Gee added a comment - This is the same change as the ci_hsc one. I would have preferred that lauren run it, since she can actually run meas_mosaic. But you can choose to review it without executing it, I believe.
            pgee Perry Gee made changes -
            Reviewers Lauren MacArthur [ lauren ] Merlin Fisher-Levine [ mfisherlevine ]
            Status In Review [ 10004 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Reviewers Merlin Fisher-Levine [ mfisherlevine ] John Swinbank [ swinbank ]
            Hide
            swinbank John Swinbank added a comment -

            Going to steal this review for myself, since I'd like to learn about the new logger. Sorry Merlin Fisher-Levine!

            Show
            swinbank John Swinbank added a comment - Going to steal this review for myself, since I'd like to learn about the new logger. Sorry Merlin Fisher-Levine !
            Hide
            swinbank John Swinbank added a comment -

            Lauren MacArthur has already pointed out that the import statements need fixing in mosaicTask.py. Other than that, this looks ok.

            Show
            swinbank John Swinbank added a comment - Lauren MacArthur has already pointed out that the import statements need fixing in mosaicTask.py . Other than that, this looks ok.
            swinbank John Swinbank made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            pgee Perry Gee added a comment -

            I'm not sure why "from log.lsst import Log" is preferred to "import log.lsst". Can someone help me here?

            Is this a matter of style, or is it desirable not to import the entire module and then refer to log as log.lsst.Log? I imported lsst.log in other tickets, probably because I thought that some of the globals in log.lsst (such as the log level constants) were potentially useful.

            Hsin-Fang Chiang, can you comment on whether there is a preferred usage?

            Show
            pgee Perry Gee added a comment - I'm not sure why "from log.lsst import Log" is preferred to "import log.lsst". Can someone help me here? Is this a matter of style, or is it desirable not to import the entire module and then refer to log as log.lsst.Log? I imported lsst.log in other tickets, probably because I thought that some of the globals in log.lsst (such as the log level constants) were potentially useful. Hsin-Fang Chiang , can you comment on whether there is a preferred usage?
            Hide
            swinbank John Swinbank added a comment -

            I don't see any compelling reason to prefer one version over the other. Either is preferred to from lsst.pex.logging import getDefaultLog, which is what you have now!

            Show
            swinbank John Swinbank added a comment - I don't see any compelling reason to prefer one version over the other. Either is preferred to from lsst.pex.logging import getDefaultLog , which is what you have now!
            Hide
            pgee Perry Gee added a comment -

            Yes, I agree that this import would be bad.

            That's strange. This line was not in the version Lauren reviewed. Her comment was about from ... import ... vs import ..., and my question was about that.

            Show
            pgee Perry Gee added a comment - Yes, I agree that this import would be bad. That's strange. This line was not in the version Lauren reviewed. Her comment was about from ... import ... vs import ..., and my question was about that.
            Hide
            swinbank John Swinbank added a comment - - edited

            I think her fundamental point was simply that your imports are wrong, because you are still importing pex_logging rather that lsst.log. She then suggested one particular change, which was based on established practice elsewhere, but isn't (as far as I know) a requirement.

            Show
            swinbank John Swinbank added a comment - - edited I think her fundamental point was simply that your imports are wrong, because you are still importing pex_logging rather that lsst.log. She then suggested one particular change, which was based on established practice elsewhere, but isn't (as far as I know) a requirement.
            Hide
            lauren Lauren MacArthur added a comment -

            Indeed...my main point was that the pex_logging import needed changing. I just pointed to what seems to be the "norm", not the "enforced" (or necessarily "recommended"!) style as far as the logging updates have gone in other packages.

            Show
            lauren Lauren MacArthur added a comment - Indeed...my main point was that the pex_logging import needed changing. I just pointed to what seems to be the "norm", not the "enforced" (or necessarily "recommended"!) style as far as the logging updates have gone in other packages.
            Hide
            pgee Perry Gee added a comment -

            I made the change Lauren ask for, though I used import lsst.log in the other tickets in this sequence.

            Show
            pgee Perry Gee added a comment - I made the change Lauren ask for, though I used import lsst.log in the other tickets in this sequence.
            pgee Perry Gee made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            Hide
            pgee Perry Gee added a comment -

            John Swinbank I think that what was in the pull request was inconsistent with what was in the ticket branch. I have a theory that I am using git incorrectly, and I will send you email to get your opinion. I think this is what is causing me to get so much trouble just before I merge to master, so that I sometimes have to end up recreating the commits on a second branch.

            Show
            pgee Perry Gee added a comment - John Swinbank I think that what was in the pull request was inconsistent with what was in the ticket branch. I have a theory that I am using git incorrectly, and I will send you email to get your opinion. I think this is what is causing me to get so much trouble just before I merge to master, so that I sometimes have to end up recreating the commits on a second branch.

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              hchiang2 Hsin-Fang Chiang
              Reviewers:
              John Swinbank
              Watchers:
              Hsin-Fang Chiang, John Swinbank, Lauren MacArthur, Merlin Fisher-Levine, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.