Interpolations for data operations #62
Conversation
|
There is a paired pull request in SasView SasView/sasview#2782 |
|
Is the plan to also push this against 0.9 which gets bundled with SasView 6.0? |
I think if the paired SasView pull request gets pushed into 6.0 then this would need to get pushed against 0.9. |
|
I will review as part of the review of reviewing SasView/sasview#2782. |
|
@butlerpd will do a functionality review on windows and check the code but this probably could use a better code review than that? |
|
I am also looking at the code as it turns out and it is unclear to me why data operations are being carried out in the Question (probably more for @krzywon and/or @lucas-wilkins or @rozyczko) : Changing that structure sounds like it would definitely be a breaking change? If so should we attempt to do it for 6.0? or do we wait for 7.0? Or am I completely misunderstanding the code structure here? If a change should eventually be made, a separate issue should be created. |
There was a problem hiding this comment.
The new interpolation code looks good to me. The math seems right and the code seems clean to the level that I can judge such things. On the other hand the changes to data-info.py I found much harder to understand as noted in the code comments.
Also I have only skimmed the new unit tests though they seem reasonable. Somebody who understands that better than I might want to check
In any event, this PR should not be merged until we understand the issues reported in the paired sasview PR which are possibly caused by this code?
| if self.isSesans: | ||
| self._interpolation_operation(other, scale='linear') |
There was a problem hiding this comment.
I think there should be a second if here: if other.isSesans then the linear interpolation otherwise raise error with message: the two data types must be the same .. or some such. Maybe more specific about not mixing sas and sesans data?
| self_overlap_bool = np.abs((self.x[:, None] - other.x[None, :]) / self.x[:, None]).min(axis=1) <= tolerance | ||
| self_overlap_index = np.flatnonzero(self_overlap_bool) | ||
| if len(self_overlap_index) == self_overlap_index.max() - self_overlap_index.min() + 1: | ||
| # all the points in overlap region of self.x found close matches in overlap region of other.x |
There was a problem hiding this comment.
I don't really understand what exactly this is doing (I'm just a poor white belt) but it does seem that the goal here was to either do no interpolation if the two data sets have exactly the same number of points AND all the points match up in x within tolerance, otherwise ALL the data will be interpolated even if the x points match up? Given the funtionality review of the paired sasview PR is this really what is happening?
| x_op = self.x[self_overlap_bool] | ||
| other._operation = other.clone_without_data(x_op.size) | ||
| other._operation.copy_from_datainfo(other) |
There was a problem hiding this comment.
so, if my understanding from the previous comment is correct, why do we need more than x_op = self.x? i.e. why is it instead x_op = self.x[self_overlap_bool]?
If it is because my understanding is incorrect, and two data sets with different sizes and q ranges can still make it through to this code if there are matching x (within tolerance), what happens when setting other._operation to be that length before copying the data from other and the size of the other data is different?
Sorry for my naive questions but I got a bit lost here.
Description
This PR adds interpolation to 1D data operations . Previously the q-values for each dataset had to match perfectly. Now the overlap range of the two datasets in q will be determined and the second dataset will be interpolated if points don't match within the specified tolerance. There is a paired PR in sasview (SasView/sasview#2782).
Fixes # (issue/issues) - I could not find the relevant issue; please comment below when found.