Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-40128: Fix brighter-fatter loading errors #279

Merged
merged 2 commits into from Sep 6, 2023
Merged

DM-40128: Fix brighter-fatter loading errors #279

merged 2 commits into from Sep 6, 2023

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Aug 29, 2023

Simplify header provenance code and fix BF errors:

  • Move calibration date searches to runQuantum with the rest of the header provenance.
  • Handle inputs that change name during the header provenance (only brighter-fatter kernels currently).
  • Retain header validation checks in the run method.

* Move calibration date searches to runQuantum with the rest of the
  header provenance.
* Handle inputs that change name during the header provenance (only
  brighter-fatter kernels currently).
* Retain header validation checks in the run method.
Copy link
Contributor

@taranu taranu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Feel free to ignore the nitpicky suggestions.


if brighterFatterKernel is None:
# This was requested by the config, but none were found.
raise RuntimeError("No brighter-fatter kernel was supplied.")

if brighterFatterKernel is not None and not isinstance(brighterFatterKernel, numpy.ndarray):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be elif not isinstance(brighterFatterKernel, numpy.ndarray) now?


# These inputs change name during this step. These should
# have matching entries in the additionalInputDates dict.
additionalInputs = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it matters much but couldn't you just set this to list(inputs.keys()) to start with and append to/extend it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would certainly work the same, but I think it would require some variable name and comment rewrites. I think in the long term, this can eventually go away if all cameras switch to IsrCalib style brighter-fatter kernels.

@czwa czwa merged commit a7ab7f6 into main Sep 6, 2023
2 checks passed
@czwa czwa deleted the tickets/DM-40128 branch September 6, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants