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

exit/raise when data is less than parameters

    Details

    • Story Points:
      2
    • Sprint:
      AP S19-1, AP S19-2, AP S19-3, AP S19-4, AP S19-5
    • Team:
      Alert Production

      Description

      Jointcal should exit or raise when the number of data points is less than the number of fitting parameters (e.g., constrained 4th order polynomial) with only 3 stars). I thought there was once such a check for astrometry, but I can't find it now.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            This is worth doing, now that we're going into production. Should be simple.

            Show
            Parejkoj John Parejko added a comment - This is worth doing, now that we're going into production. Should be simple.
            Hide
            Parejkoj John Parejko added a comment -

            Meredith Rawls: can you please review this medium sized change (~100 lines), involving a little C++? One of the commits is just cleaning up and expanding on some unrelated docstrings, so you might want to review the commits separately.

            In particular, I have questions about the correct minimum value of ndof (number of degrees of freedom) for photometry vs. astrometry: I'm positive it's 1 for the former, since I have an explicitly computed model with exactly 1 dof, but it might be 2 for the latter since it's fitting x,y, but I'm not sure how to think about this properly. I'm also not positive how to word the resulting error message if the checkfails. The docstring for the new validate() methods refer to FitterBase.computeChi2:

            https://github.com/lsst/jointcal/blob/master/src/FitterBase.cc#L42

            Looking at it, I should probably also improve its documentation to clarify that it returns the "reduced" degrees of freedom (data - parameters): sadly, I don't know what the correct terminology for this is.

            Show
            Parejkoj John Parejko added a comment - Meredith Rawls : can you please review this medium sized change (~100 lines), involving a little C++? One of the commits is just cleaning up and expanding on some unrelated docstrings, so you might want to review the commits separately. In particular, I have questions about the correct minimum value of ndof (number of degrees of freedom) for photometry vs. astrometry: I'm positive it's 1 for the former, since I have an explicitly computed model with exactly 1 dof, but it might be 2 for the latter since it's fitting x,y, but I'm not sure how to think about this properly. I'm also not positive how to word the resulting error message if the checkfails. The docstring for the new validate() methods refer to FitterBase.computeChi2 : https://github.com/lsst/jointcal/blob/master/src/FitterBase.cc#L42 Looking at it, I should probably also improve its documentation to clarify that it returns the "reduced" degrees of freedom (data - parameters): sadly, I don't know what the correct terminology for this is.
            Hide
            mrawls Meredith Rawls added a comment -

            The doctoring cleanups and expansions are great, thank you!

            I did some brief refresher-googling about chi2 and degrees of freedom. DOF is typically defined as the number of independent observations or measurements minus the number of fitted parameters. So the Chi2Statistic in FitterBase is fine as-is.

            I didn't look closely enough at your astrometry model to comment on the "correct" number of DOF. Your wording presently is "Not enough degrees of freedom [ndof] to fit model with getTotalParameters()." It's not entirely clear to me where getTotalParameters comes from, but a clearer message could be, "Fitting the astrometry/photometry model requires at least 1 degree of freedom, and you have supplied only X data points - Y parameters = Z degrees of freedom. [BONUS: Relevant info about how to fix this here.]"

            Show
            mrawls Meredith Rawls added a comment - The doctoring cleanups and expansions are great, thank you! I did some brief refresher-googling about chi2 and degrees of freedom. DOF is typically defined as the number of independent observations or measurements minus the number of fitted parameters. So the Chi2Statistic in FitterBase is fine as-is. I didn't look closely enough at your astrometry model to comment on the "correct" number of DOF. Your wording presently is "Not enough degrees of freedom [ndof] to fit model with getTotalParameters()." It's not entirely clear to me where getTotalParameters comes from, but a clearer message could be, "Fitting the astrometry/photometry model requires at least 1 degree of freedom, and you have supplied only X data points - Y parameters = Z degrees of freedom. [BONUS: Relevant info about how to fix this here.] "
            Hide
            Parejkoj John Parejko added a comment - - edited

            I've updated the error message so that a failure will now read like this in the log:

            jointcal.ConstrainedAstrometryModel ERROR: Fitting this model requires at least 1 degree of freedom but only 0 available, with 70 total parameters. Reduce the model complexity (e.g. polynomial order) to better match the number of measured sources.

            Show
            Parejkoj John Parejko added a comment - - edited I've updated the error message so that a failure will now read like this in the log: jointcal.ConstrainedAstrometryModel ERROR: Fitting this model requires at least 1 degree of freedom but only 0 available, with 70 total parameters. Reduce the model complexity (e.g. polynomial order) to better match the number of measured sources.
            Hide
            mrawls Meredith Rawls added a comment -

            Looks great! I'll mark this as reviewed now, but I'm happy to chat more about how many dof astrometry really needs if you'd like.

            Show
            mrawls Meredith Rawls added a comment - Looks great! I'll mark this as reviewed now, but I'm happy to chat more about how many dof astrometry really needs if you'd like.
            Hide
            Parejkoj John Parejko added a comment -

            Thank you for the comments, Meredith Rawls.

            I chatted with Jim Bosch about this today, and he convinced me that the correct number is zero: a model with zero degrees of freedom has a matrix with nRows==nColums, which means it is fully determined. That doesn't necessarily guarantee that a solution exists (i.e. the matrix might not have an inverse), but it is the absolute minimum requirement, which is what this code is testing for. In the future, we could add a config parameter to allow the user to make this requirement higher (in reality, we want a lot more degrees of freedom!), but for now this is enough. I've updated the code to reflect this, and tried to clarify the error messages a bit more.

            Merged and done.

            Show
            Parejkoj John Parejko added a comment - Thank you for the comments, Meredith Rawls . I chatted with Jim Bosch about this today, and he convinced me that the correct number is zero: a model with zero degrees of freedom has a matrix with nRows==nColums , which means it is fully determined. That doesn't necessarily guarantee that a solution exists (i.e. the matrix might not have an inverse), but it is the absolute minimum requirement, which is what this code is testing for. In the future, we could add a config parameter to allow the user to make this requirement higher (in reality, we want a lot more degrees of freedom!), but for now this is enough. I've updated the code to reflect this, and tried to clarify the error messages a bit more. Merged and done.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Meredith Rawls
                Watchers:
                John Parejko, John Swinbank, Meredith Rawls
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel