Add an accepted column so a usable fit reads as a predicate - #527
Open
matthewholman wants to merge 1 commit into
Open
Add an accepted column so a usable fit reads as a predicate#527matthewholman wants to merge 1 commit into
matthewholman wants to merge 1 commit into
Conversation
Reviewers on #495 asked for a status column that reads as `if (!error)` or `if (success)` rather than an integer compared against a documented list. The two are not interchangeable -- 0 means converged today, so `!error` works with the values unchanged while `success` is false for every good fit unless the values also invert -- and renaming `flag` is breaking: seven fixture CSVs carry it as a column header, so output already written stops loading. This is #498's alternative instead: keep `flag`, add a derived boolean. `accepted` is 1 exactly when `flag == FLAG_CONVERGED`. It reads the same, inverts nothing, and does not label -1 (never attempted) or 8 (incremental bookkeeping) as errors, which they are not. It is derived from the flag the row carries rather than tracked beside the other outcome facts, so the two cannot disagree, and it is taken after any stage marker has overwritten the fitter's own verdict -- that overwritten value is what a reader sees. Also replaces the hand-copied outcome tuple in create_empty_result with a real as_row call. It was a second copy of the column layout and would have silently desynced; the existing tests caught it as a length mismatch. Separately, corrects the failed_cov entry in docs/fit_flags.rst. Both of its assignments sit inside the non-gravitational weak-constraint guard, so a gravity-only fit is never marked there -- the page implied the covariance was examined for every fit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #498.
Reviewers on #495 asked for a status column that reads as a predicate --
if (!error)orif (success)-- rather than an integer compared against a documented list.The two are not interchangeable:
0means converged today, so!errorworks with the values unchanged whilesuccessis false for every good fit unless the values also invert. And renamingflagis breaking -- seven fixture CSVs carry it as a column header, so output already written stops loading.This is the alternative from #498 instead: keep
flag, add a derived boolean.acceptedis 1 exactly whenflag == 0. It reads the same, inverts nothing, and does not label -1 (never attempted) or 8 (incremental bookkeeping) as errors, which they are not.It is derived from the flag the row carries rather than tracked beside the other outcome facts, so the two cannot disagree, and it is taken after any stage marker has overwritten the fitter's own verdict -- that overwritten value is what a reader sees.
Two things came along with it:
create_empty_resulthand-copied the outcome tuple, a second copy of the column layout that would have silently desynced. It now callsas_row.failed_coventry indocs/fit_flags.rst. Both of its assignments sit inside the non-gravitational weak-constraint guard, so a gravity-only fit is never marked there. The page implied the covariance was examined for every fit, which would let a reader take a column of zeros as reassurance.