Spectral adaptive samplers#338
Conversation
… adaptive samplers.
|
Wow, had a quick look through and its really nice code Vlad. I think its one Alex would probably want to review. |
CnlPepper
left a comment
There was a problem hiding this comment.
Thanks for fixing up some of the existing classes, they were in need of completing. I have the following general comments I'd like to see addressed before these are merged, in addition to the specific comments in the code:
-
I've generally only highlighted a single instance of a change in the code comments, please apply those comments to all equivalent instances in the merge request.
-
It seems unnecessary to have Masked and non-Masked samplers. Just add masking to the existing samplers and ignore the mask if it is set to None (default). Easy to implement, just an 'if mask and mask[i][j]:' test at the point of evaluation.
-
Some of the functions are getting overlong.
-
Don't force users to supply data in a specific form when it is trivial to handle and convert consistently.
Otherwise, looks good. I like the choice of weighting methods for the spectral samplers.
If you can make these changes I'll merge them for the next release.
| @fraction.setter | ||
| def fraction(self, value): | ||
| if value <= 0 or value > 1.: | ||
| raise ValueError("fraction must be in the range (0, 1]") |
There was a problem hiding this comment.
Legibility: All error messages should be proper sentences. Start with a capital letter and end with a full stop.
| def mask(self, np.ndarray value): | ||
| if value.ndim != 2: | ||
| raise ValueError("Mask must be a 2D array") | ||
| if value.dtype not in (np.int, np.int32, np.int64, np.bool): |
There was a problem hiding this comment.
User convenience: I would just convert the mask array to dtype=np.bool and state that in the document. Any value != to 0 will map to True.
| def mask(self, np.ndarray value): | ||
| if value.ndim != 2: | ||
| raise ValueError("Mask must be a 2D array") | ||
| if value.dtype not in (np.int, np.int32, np.int64, np.bool): |
There was a problem hiding this comment.
See previous comment on masks.
| normalised_mv = normalised | ||
|
|
||
| # calculated normalised standard error | ||
| if self.reduction_method == 'weighted': |
There was a problem hiding this comment.
These options should really be in their own functions, or made plugable classes (maybe a later improvement).
There was a problem hiding this comment.
I moved them to their own functions.
|
@CnlPepper, thank you very much for the review. I hope that I fixed all the issues you indicated. Below are some comments.
|
|
There was a small formatting error in the docstring of Spectral Adaptive samplers which is fixed now. |
CnlPepper
left a comment
There was a problem hiding this comment.
Like the new improvements, just spotted 2 more items to consider then I'll merge.
…d unnecessary reassignments of memoryview objects.
|
Thanks for the work on this. There are a few tweaks I'll make the the mask interface to make it more user friendly, but I'll leave them until we get closer to release. At present, if you rerun a job with a different pixel size the old mask will be used and error at the frame size check. |
|
Yes, this is a bug. If the mask was not specified by the user, the error should not be raised. Probably the easiest fix is to replace: with |
Spectral adaptive samplers
This adds the following new samplers:
MaskedFrameSampler2D,SpectralAdaptiveSampler1D,SpectralAdaptiveSampler2D,MaskedSpectralAdaptiveSampler2D.Also, parameter validation is added for all adaptive samplers (I hope this doesn't break anything).
SpectralAdaptiveSampler2Dis useful when calculating in a single run multiple filtered camera images with different filters. It has three options for how to reduce the spectral array of normalized errors to a single value for a pixel. I'll be happy to add more options if needed.There are also two small changes in documentation:
ratioparameter of adaptive samplers is added,