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

Convert old refcats to nJy for RFC-549

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      As fallout from RFC-549, we have to transition our existing reference catalogs from having fluxes in Jansky (most only implicitly: most don't have units on their flux fields at all!) to nanoJansky. We do not have versioning for reference catalogs (the afw.table version has a different purpose and is not propagated to the butler), so we have no way to identify "old" refcats other than attempting to guess from the various fields they do or don't contain.

      I propose to do this by writing a script that will convert refcat files to have nJy units and having LoadReferenceObjectsTask.loadSkyCircle raise an exception pointing to that script if it encounters a refcat that has anything other than "nJy" for flux units. This will greatly simplify this transition and will allow us to also fix the fluxSigma->fluxErr values in those refcats, while maintaining the old refcats in-place for older versions of the stack.

      For example, on lsst-dev, we would copy each of the existing directories in /datasets/refcats/htm, appending "-nJy"; run the script on those new directories; and update the obs package config defaults to point to the new refcats. Those old refcats could then be kept in place until at least v18.

      All newly created refcats will have fluxes in units of nJy.

      This would be made much easier if we could drop support for astrometry.net reference catalogs at the same time (RFC-562): a.net refcats are persisted in a way that makes them much harder to modify in-place. I believe the only important a.net refcat that we are still using is in testdata_jointcal, and I have a plan for dealing with it soon.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment - - edited

            Flagging this for attention by the DM-CCB.

            The DM-CCB should note in particular that:

            • The transition from Jy to nJy has been accepted on RFC-549. We should not reopen that discussion. The questions here are about mechanism and timing.
            • The proposed approach means that using old reference catalogs with new code would be an error, and vice versa. There's no obvious way to signal this as a deprecation, and we can't reliably produce code that can read both versions at once. In short, this will be a disruptive change to all users with their own reference catalogs: they will be obliged to convert them to the new format to use new versions of the Pipelines.
            • Input from the DM-CCB on timing would be particularly valuable. It's been suggested that making this change as soon as possible after the v17 release would be convenient for end users — does the DM-CCB agree?
            Show
            swinbank John Swinbank added a comment - - edited Flagging this for attention by the DM-CCB. The DM-CCB should note in particular that: The transition from Jy to nJy has been accepted on RFC-549 . We should not reopen that discussion. The questions here are about mechanism and timing. The proposed approach means that using old reference catalogs with new code would be an error, and vice versa. There's no obvious way to signal this as a deprecation, and we can't reliably produce code that can read both versions at once. In short, this will be a disruptive change to all users with their own reference catalogs: they will be obliged to convert them to the new format to use new versions of the Pipelines. Input from the DM-CCB on timing would be particularly valuable. It's been suggested that making this change as soon as possible after the v17 release would be convenient for end users — does the DM-CCB agree?
            Hide
            swinbank John Swinbank added a comment -

            Worth adding that, although this is flagged for the DM-CCB to look at, we should also solicit other suggested approaches — if anybody can suggest a less disruptive approach, that would certainly be interesting.

            Show
            swinbank John Swinbank added a comment - Worth adding that, although this is flagged for the DM-CCB to look at, we should also solicit other suggested approaches — if anybody can suggest a less disruptive approach, that would certainly be interesting.
            Hide
            jbosch Jim Bosch added a comment -

            I don't currently have any ideas for a less disruptive approach, though I haven't thought deeply about the problem.

            Apologies for what may be a silly question (or at least one that will reveal how little I've been following the state of reference catalogs), but are there any other changes we are prepared to make now (or very soon) that we could bundle with these, so we can at least keep the number of flag days to a minimum?  I don't actually recall whether all of the proper-motion changes have been proposed have already landed, for example.

            Show
            jbosch Jim Bosch added a comment - I don't currently have any ideas for a less disruptive approach, though I haven't thought deeply about the problem. Apologies for what may be a silly question (or at least one that will reveal how little I've been following the state of reference catalogs), but are there any other changes we are prepared to make now (or very soon) that we could bundle with these, so we can at least keep the number of flag days to a minimum?  I don't actually recall whether all of the proper-motion changes have been proposed have already landed, for example.
            Hide
            price Paul Price added a comment -

            When I wrote the draft implementation of the proper motion reading, I included a translation layer that would allow existing catalogs to work with the new format. I don't know what happened to that (I suspect it was dropped), but surely that sort of approach would keep the fuss caused by catalog turnover to a minimum. We're just talking multiplicative factors for the flux columns, so the translation layer would be really simple, and wouldn't add much complexity to the code.

            Show
            price Paul Price added a comment - When I wrote the draft implementation of the proper motion reading, I included a translation layer that would allow existing catalogs to work with the new format. I don't know what happened to that (I suspect it was dropped), but surely that sort of approach would keep the fuss caused by catalog turnover to a minimum. We're just talking multiplicative factors for the flux columns, so the translation layer would be really simple, and wouldn't add much complexity to the code.
            Hide
            Parejkoj John Parejko added a comment -

            Are there any other changes we are prepared to make?

            We could use it as an opportunity to fully implement RFC-535 (make fluxErr a required column). The proper motion-related changes only apply to catalogs that have proper motions in them, which most of our existing refcats do not. Unless there were changes to the names of proper motion columns, which Russell Owen would probably have to chime in on?

            We're just talking multiplicative factors for the flux columns, so the translation layer would be really simple, and wouldn't add much complexity to the code.

            Multiplicative factors, plus remapping the schema (which means copying the catalog) to have the units field be correct.

            Show
            Parejkoj John Parejko added a comment - Are there any other changes we are prepared to make? We could use it as an opportunity to fully implement RFC-535 (make fluxErr a required column). The proper motion-related changes only apply to catalogs that have proper motions in them, which most of our existing refcats do not. Unless there were changes to the names of proper motion columns, which Russell Owen would probably have to chime in on? We're just talking multiplicative factors for the flux columns, so the translation layer would be really simple, and wouldn't add much complexity to the code. Multiplicative factors, plus remapping the schema (which means copying the catalog) to have the units field be correct.
            Hide
            swinbank John Swinbank added a comment -

            We're just talking multiplicative factors for the flux columns, so the translation layer ... (etc)

            Given that we're told (in the RFC) that most catalogs don't actually record their units, how would you know what conversion to apply?

            On some level, that's a silly question — obviously, you can make an informed guess whether you're dealing with Jy or nJy based on how large the value is — but that sort of heuristic seems like both a hack and a slippery slope to me. Is there a bulletproof way to do this sort of inference?

            Show
            swinbank John Swinbank added a comment - We're just talking multiplicative factors for the flux columns, so the translation layer ... (etc) Given that we're told (in the RFC) that most catalogs don't actually record their units, how would you know what conversion to apply? On some level, that's a silly question — obviously, you can make an informed guess whether you're dealing with Jy or nJy based on how large the value is — but that sort of heuristic seems like both a hack and a slippery slope to me. Is there a bulletproof way to do this sort of inference?
            Hide
            Parejkoj John Parejko added a comment -

            In the existing code, we have been assuming that all refcats are in Jy, whether they have a units field or not. This is one advantage to the method proposed in this RFC: we would enforce the units field when loading refcats in the future to avoid the necessity for any such assumptions.

            As part of RFC-549, I am making LoadReferenceObjectsTask.makeMinimalSchema list units of nJy for flux fields. Of course, many of our existing refcats were not created with this function, so unless we enforce it when reading refcats as above, we will have a worse problem in the future.

            I will repeat here what I argued on RFC-535: most of our existing refcats (e.g. SDSS, Gaia DR1) should be thrown out soon and replaced with Gaia DR2 for astrometry and PS1 DR2 for photometry. Once we have those, there will be little need to go back to the old refcats, with their variety of oddities.

            Show
            Parejkoj John Parejko added a comment - In the existing code, we have been assuming that all refcats are in Jy, whether they have a units field or not. This is one advantage to the method proposed in this RFC: we would enforce the units field when loading refcats in the future to avoid the necessity for any such assumptions. As part of RFC-549 , I am making LoadReferenceObjectsTask.makeMinimalSchema list units of nJy for flux fields. Of course, many of our existing refcats were not created with this function, so unless we enforce it when reading refcats as above, we will have a worse problem in the future. I will repeat here what I argued on RFC-535 : most of our existing refcats (e.g. SDSS, Gaia DR1) should be thrown out soon and replaced with Gaia DR2 for astrometry and PS1 DR2 for photometry. Once we have those, there will be little need to go back to the old refcats, with their variety of oddities.
            Hide
            Parejkoj John Parejko added a comment -

            For the record, I've pushed an example script that accomplishes the conversion job, and is parallelized so it should run in a finite amount of time on e.g. our 500GB PS1 catalog:

            https://github.com/lsst/meas_algorithms/blob/tickets/DM-17029/bin.src/convert_refcat_to_nJy.py

            Show
            Parejkoj John Parejko added a comment - For the record, I've pushed an example script that accomplishes the conversion job, and is parallelized so it should run in a finite amount of time on e.g. our 500GB PS1 catalog: https://github.com/lsst/meas_algorithms/blob/tickets/DM-17029/bin.src/convert_refcat_to_nJy.py
            Hide
            lguy Leanne Guy added a comment -

            I agree that this is the least disruptive approach.  There is the potential for this to impact DESC DC2 processing, I want to review the timeline to implement this with DESC at the collaboration meeting next week. Deferring decision until the next DM-CCB 

            Show
            lguy Leanne Guy added a comment - I agree that this is the least disruptive approach.  There is the potential for this to impact DESC DC2 processing, I want to review the timeline to implement this with DESC at the collaboration meeting next week. Deferring decision until the next DM-CCB 
            Hide
            swinbank John Swinbank added a comment -

            (I note for John Parejko's — and others' — information that the next DM-CCB meeting will be on 2019-03-06.)

            Show
            swinbank John Swinbank added a comment - (I note for John Parejko 's — and others' — information that the next DM-CCB meeting will be on 2019-03-06.)
            Hide
            price Paul Price added a comment -

            Given that we're told (in the RFC) that most catalogs don't actually record their units, how would you know what conversion to apply?

            Each catalog knows what its units are, and can easily be updated to provide a Config file specifying the requisite conversion factors. That's much easier than rewriting the catalogs.

            Show
            price Paul Price added a comment - Given that we're told (in the RFC) that most catalogs don't actually record their units, how would you know what conversion to apply? Each catalog knows what its units are, and can easily be updated to provide a Config file specifying the requisite conversion factors. That's much easier than rewriting the catalogs.
            Hide
            jbosch Jim Bosch added a comment - - edited

            I think I agree with Paul Price that it would be possible to create an adapter layer that would make rewriting all of these files unnecessary - perhaps more accurately, I have no reason to dispute his claim, and I haven't yet heard anyone who would know more than me (John Parejko? Simon Krughoff?) provide a good reason why it wouldn't work in this case.

            But such a layer has a cost, too, in terms of additional code complexity and maintenance, and I don't know how that compares to the cost of a flag-day switchover.  If we do go with an adapter layer, I would like to at least explicitly deprecate the current on-disk format at the same time so there's an understanding that we won't be maintaining that layer indefinitely.

            We have erred in both directions in the past (lack of stability and too much adapter-layer maintenance), IMO by not having clearly-defined deprecation periods for on-disk data products.  Part of the reason I am not completely opposed to a flag-day in this case is that I think we've already got a fair amount of adapter-layer cruft in this part of the codebase.

            Show
            jbosch Jim Bosch added a comment - - edited I think I agree with Paul Price that it would be possible to create an adapter layer that would make rewriting all of these files unnecessary - perhaps more accurately, I have no reason to dispute his claim, and I haven't yet heard anyone who would know more than me ( John Parejko ? Simon Krughoff ?) provide a good reason why it wouldn't work in this case. But such a layer has a cost, too, in terms of additional code complexity and maintenance, and I don't know how that compares to the cost of a flag-day switchover.  If we do go with an adapter layer, I would like to at least explicitly deprecate the current on-disk format at the same time so there's an understanding that we won't be maintaining that layer indefinitely. We have erred in both directions in the past (lack of stability and too much adapter-layer maintenance), IMO by not having clearly-defined deprecation periods for on-disk data products.  Part of the reason I am not completely opposed to a flag-day in this case is that I think we've already got a fair amount of adapter-layer cruft in this part of the codebase.
            Hide
            lguy Leanne Guy added a comment -

            Users with their own reference catalogs would also have to update their catalogs to provide a config file. 

            Show
            lguy Leanne Guy added a comment - Users with their own reference catalogs would also have to update their catalogs to provide a config file. 
            Hide
            price Paul Price added a comment -

            But a config file is a tiny fraction of the size of an entire catalog!

            I am willing to believe there's an argument to dump the old catalogs, but that's not it.

            Show
            price Paul Price added a comment - But a config file is a tiny fraction of the size of an entire catalog! I am willing to believe there's an argument to dump the old catalogs, but that's not it.
            Hide
            krughoff Simon Krughoff added a comment - - edited

            My argument against an adapter layer was the dual work of creating that layer and then removing it. I also know, from experience, that removing code seems to be harder than adding it. It seemed at the time I was discussing with John Parejko that translating the catalogs we know about and providing tools for users to do so on their own has the least overall overhead. We need to do that work eventually anyway.

            I understand the hesitance, but it really does seem cleaner to me to make v17 be the last version guaranteed to support the old units.

            I do think this basically adheres to our code deprecation which as I understand is:
            1) announce deprecation
            2) make a release without deprecation
            3) work toward deprecation
            4) fully deprecate in the following release.

            Show
            krughoff Simon Krughoff added a comment - - edited My argument against an adapter layer was the dual work of creating that layer and then removing it. I also know, from experience, that removing code seems to be harder than adding it. It seemed at the time I was discussing with John Parejko that translating the catalogs we know about and providing tools for users to do so on their own has the least overall overhead. We need to do that work eventually anyway. I understand the hesitance, but it really does seem cleaner to me to make v17 be the last version guaranteed to support the old units. I do think this basically adheres to our code deprecation which as I understand is: 1) announce deprecation 2) make a release without deprecation 3) work toward deprecation 4) fully deprecate in the following release.
            Hide
            ctslater Colin Slater added a comment -

            I don't think flag day counts as deprecation process; the key is having an overlap period where both methods work.

            Even if we thought we could get away with a disruptive change this time, I think it would be much better to use this as a chance to institute a file versioning process. We're going to want to make more changes like this in the future, and it will be a problem if we always feel like a big-bang change is the only way to do it.

            I would be much happier with something like:

            • We change the refcat creation code to write nJy in the units fields, and add an entry in the refcat config (the file that stores things like the HTM level and indexing method) that specifies this is refcat format 1.
            • The refcat loader and tasks that call it are changed use nJy, and if the loader reads a refcat without the version string it assumes the catalog is in Jy (refcat format 0), outputs appropriate warnings to the user, and does the conversion to output nJy to the calling task.
            • After this is merged, we can convert our on-disk refcats. This doesn't need to be done urgently, since old and new formats are supported.
            • After ~6 months or so, we remove support for refcat format 0. A second RFC is not required, but we will need to notify users.
            Show
            ctslater Colin Slater added a comment - I don't think flag day counts as deprecation process; the key is having an overlap period where both methods work. Even if we thought we could get away with a disruptive change this time, I think it would be much better to use this as a chance to institute a file versioning process. We're going to want to make more changes like this in the future, and it will be a problem if we always feel like a big-bang change is the only way to do it. I would be much happier with something like: We change the refcat creation code to write nJy in the units fields, and add an entry in the refcat config (the file that stores things like the HTM level and indexing method) that specifies this is refcat format 1. The refcat loader and tasks that call it are changed use nJy, and if the loader reads a refcat without the version string it assumes the catalog is in Jy (refcat format 0), outputs appropriate warnings to the user, and does the conversion to output nJy to the calling task. After this is merged, we can convert our on-disk refcats. This doesn't need to be done urgently, since old and new formats are supported. After ~6 months or so, we remove support for refcat format 0. A second RFC is not required, but we will need to notify users.
            Hide
            swinbank John Swinbank added a comment -

            I don't think flag day counts as deprecation process; the key is having an overlap period where both methods work.

            I broadly agree with that. My understanding is the request for a “flag day” transition is effectively a request to circumvent the deprecation process. I think that's permissible, as long as the DM-CCB agrees.

            If we were to decide to implement an adapter, I don't see an argument for not following the procedure in the Developer Guide. Specifically, that would mean reading (and inferring units for) version “0“ catalogs while issuing a deprecation warnings for (at least) one release, then dropping support for those catalogs on the next major release.

            We're within a gnat's crotchet of the 17.0 release and those deprecation warnings are not currently being issued. I don't think it's plausible to suggest that we'll hold the release for them. That means that we'd either have to include deprecation notices in 17.1 (or 18.0) and then not drop support for version 0 catalogs until after the 18.0 (or 19.0) release.

            Given that timeline, I personally prefer making a clean break and just dropping the legacy code. However, I'm sympathetic to Colin's point; if he or others believe that will cause major inconvenience to external users, then I think that's a serious concern.

            Show
            swinbank John Swinbank added a comment - I don't think flag day counts as deprecation process; the key is having an overlap period where both methods work. I broadly agree with that. My understanding is the request for a “flag day” transition is effectively a request to circumvent the deprecation process. I think that's permissible, as long as the DM-CCB agrees. If we were to decide to implement an adapter, I don't see an argument for not following the procedure in the Developer Guide . Specifically, that would mean reading (and inferring units for) version “0“ catalogs while issuing a deprecation warnings for (at least) one release, then dropping support for those catalogs on the next major release. We're within a gnat's crotchet of the 17.0 release and those deprecation warnings are not currently being issued. I don't think it's plausible to suggest that we'll hold the release for them. That means that we'd either have to include deprecation notices in 17.1 (or 18.0) and then not drop support for version 0 catalogs until after the 18.0 (or 19.0) release. Given that timeline, I personally prefer making a clean break and just dropping the legacy code. However, I'm sympathetic to Colin's point; if he or others believe that will cause major inconvenience to external users, then I think that's a serious concern.
            Hide
            Parejkoj John Parejko added a comment -

            To try to move this along, I've implemented some "check and convert" code in loadReferenceObjectsTask, and refactored the conversion script I wrote to use it. It still needs some tests of the Indexed ref loader, but the commandline code behaves as expected. This code does not use a version identifier, but instead checks the existing flux fields for whether they have `nJy` units. If someone could help me implement the config versioning that Colin Slater describes above, I think we could have the behavior that Colin Slater describes.

            Take a look at the branch:

            https://github.com/lsst/meas_algorithms/compare/tickets/DM-17029#diff-282f082f7e9aa1fd4b2eddad9048d760

            Show
            Parejkoj John Parejko added a comment - To try to move this along, I've implemented some "check and convert" code in loadReferenceObjectsTask , and refactored the conversion script I wrote to use it. It still needs some tests of the Indexed ref loader, but the commandline code behaves as expected. This code does not use a version identifier, but instead checks the existing flux fields for whether they have `nJy` units. If someone could help me implement the config versioning that Colin Slater describes above, I think we could have the behavior that Colin Slater describes. Take a look at the branch: https://github.com/lsst/meas_algorithms/compare/tickets/DM-17029#diff-282f082f7e9aa1fd4b2eddad9048d760

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Watchers:
              Colin Slater, Gabriele Comoretto [X] (Inactive), Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Leanne Guy, Margaret Gelman, Meredith Rawls, Paul Price, Robert Lupton, Simon Krughoff, Tim Jenness, Wil O'Mullane, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins

                  No builds found.