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

defaultFilter is not used if a filterName is given to loadSkyCircle

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_algorithms
    • Labels:
      None
    • Story Points:
      2
    • Sprint:
      AP F20-2 (July)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      If defaultFilter is supplied without a filterMap, and the user specifies filter=something in loadSkyCircle() (as one normally does, because when you load the refcat you don't know how its filter mappings are configured), getRefFluxField() will fail:

      RuntimeError: Could not find flux field(s) r_camFlux, r_flux
      

      I believe the solution is to add camFlux to the fluxFieldList in getRefFluxField(), so that if defaultFilter is specified, you'll still get a fluxField of some kind. The behavior of how defaultFilter and filterMap should interact is undefined in the docs that I can see, so although this is a behavior change, I think it is more self-consistent. We also do not use defaultFilter anywhere in the stack currently that I've found, and this change should make it useable for RFC-697.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment - - edited

            Based on these questions, I feel like we need to clarify some things in the refcat loader documentation, because it seems like there are a few different ideas about how this should work.

            > I believe the original intent was that you would only specify defaultFilter if there was a filterMap.

            This doesn't make sense to me: if you have a filterMap, you only need a defaultFilter if you "missed" a filter key, which we never do. As used, we have no defaultFilter​s defined anywhere in the stack at all. If this was the intent, what is the use case for defaultFilter?

            > Not providing a filterMap seems to be asking for trouble.

            How so? The intent here is to have every filter map to Gaia G, regardless of telescope or instrument. That is a broad band filter, and the most reasonable filter choice for any astrometric question related to Gaia. Having to do that via filterMap is more complication than seems warranted.

            > I'm still puzzled why one specifies a default filter in this situation. I would have thought it best to specify neither a filter map or a default filter.

            If you don't specify a defaultFilter or filterMap, the refcat loader will fail because there is no way to choose a fluxField. Specifying defaultFilter is the minimum requirement for loading a refcat, regardless of whether or not you provide filterName to loadSkyCircle. filterMap allows more flexibility and can completely supplant defaultFilter if you provide all your filter names as keys, but for cases like this seems unnecessarily complicated. Hence this ticket.

            As Jim says, DCR doesn't really care about your choice of refcat filter, because the reference catalog positions should have been normalized (or in Gaia, have no DCR at all), but your own calculations have to include the DCR math, which depends on what's going on within your own bandpass and the on-sky position of your observation.

            Show
            Parejkoj John Parejko added a comment - - edited Based on these questions, I feel like we need to clarify some things in the refcat loader documentation, because it seems like there are a few different ideas about how this should work. > I believe the original intent was that you would only specify defaultFilter if there was a filterMap. This doesn't make sense to me: if you have a filterMap , you only need a defaultFilter if you "missed" a filter key, which we never do. As used, we have no defaultFilter ​s defined anywhere in the stack at all. If this was the intent, what is the use case for defaultFilter ? > Not providing a filterMap seems to be asking for trouble. How so? The intent here is to have every filter map to Gaia G, regardless of telescope or instrument. That is a broad band filter, and the most reasonable filter choice for any astrometric question related to Gaia. Having to do that via filterMap is more complication than seems warranted. > I'm still puzzled why one specifies a default filter in this situation. I would have thought it best to specify neither a filter map or a default filter. If you don't specify a defaultFilter or filterMap , the refcat loader will fail because there is no way to choose a fluxField . Specifying defaultFilter is the minimum requirement for loading a refcat, regardless of whether or not you provide filterName to loadSkyCircle . filterMap allows more flexibility and can completely supplant defaultFilter if you provide all your filter names as keys, but for cases like this seems unnecessarily complicated. Hence this ticket. As Jim says, DCR doesn't really care about your choice of refcat filter, because the reference catalog positions should have been normalized (or in Gaia, have no DCR at all), but your own calculations have to include the DCR math, which depends on what's going on within your own bandpass and the on-sky position of your observation.
            Hide
            rowen Russell Owen added a comment - - edited

            After discussion on slack we seem to have agreed not to use defaultFilter for this purpose, but instead, add a new configuration parameter tentatively called anyFilterMapsToThis that acts as a trivial filter map. It will be an error to specify both a filter map and this new configuration parameter.

            John Parejko also filed RFC-716 to remove the defaultFilter parameter. That change, if approved, is likely to be implemented on a separate ticket branch.

            Show
            rowen Russell Owen added a comment - - edited After discussion on slack we seem to have agreed not to use defaultFilter for this purpose, but instead, add a new configuration parameter tentatively called anyFilterMapsToThis that acts as a trivial filter map. It will be an error to specify both a filter map and this new configuration parameter. John Parejko also filed RFC-716 to remove the defaultFilter parameter. That change, if approved, is likely to be implemented on a separate ticket branch.
            Show
            Parejkoj John Parejko added a comment - - edited New Jenkins run:  https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/32360/pipeline/
            Hide
            Parejkoj John Parejko added a comment -

            Russell Owen: I've reworked it based on our conversation, and reverted the defaultfilter changes (I hope). Please take another look, and note that there is a new PR for a one-line addition to obs_subaru, to disable the anyFilterMapsToThis config because HSC is still using PS1 (see DM-25849 for when that will change).

            Show
            Parejkoj John Parejko added a comment - Russell Owen : I've reworked it based on our conversation, and reverted the defaultfilter changes (I hope). Please take another look, and note that there is a new PR for a one-line addition to obs_subaru, to disable the anyFilterMapsToThis config because HSC is still using PS1 (see DM-25849 for when that will change).
            Hide
            rowen Russell Owen added a comment -

            This looks great. A very useful addition.

            Show
            rowen Russell Owen added a comment - This looks great. A very useful addition.

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              Russell Owen
              Watchers:
              Jim Bosch, John Parejko, John Swinbank, Paul Price, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.