DM-39842: Convert MeasureApCorr to new exceptions #364
Conversation
parejkoj
force-pushed
the
tickets/DM-39842
branch
2 times, most recently
from
a8dff61
to
579bcb1
Compare
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
579bcb1
to
722d144
Compare
| "ndof": self.ndof, | ||
| } | ||
| # NOTE: have to do this because task metadata doesn't allow None. | ||
| if self.iteration is not None: |
There was a problem hiding this comment.
Is the is not None part needed here? I feel like flake8 has yelled at me in the past for not just doing, e.g., if thing instead of if thing is not None
There was a problem hiding this comment.
You're thinking of if thing == None, which is not good (None is a singleton). In this case, the check is explicitly "is this None", not any other "falsey" value.
| # We now try again after declaring that the aperture correction is | ||
| # allowed to fail. This should run cleanly without raising an exception. | ||
| # allowed to fail. This should run without raising an exception, but | ||
| # will log a warning. |
There was a problem hiding this comment.
This is a really nice example of the utility of this new exception handling option! However, it's not clear to me how a user would set this at runtime, do you have an example for how to do that available somewhere?
There was a problem hiding this comment.
This is specifically for the aperture correction algorithm: allowFailure is the config option that sets which measurements are allowed to fail. It's a feature that DRP uses, but I don't know anything about it. I'm just making the existing tests support the new exception system.
There was a problem hiding this comment.
Right, I know this is a specific situation where you now use allowFailure, and thus here lies the one place where all of this began to make sense to me. 😄
There was a problem hiding this comment.
No, allowFailure has always been in MeasureApCorr, it has nothing to do with my changes. All the change here does is make the test pass, since the details of the exception have changed.
The new Task exceptions contain self-describing metadata, so we don't have to also log a warning when we raise them.
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
722d144
to
7fa028a
Compare
No description provided.