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

Interpolation for operations #2782

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Conversation

caitwolf
Copy link
Contributor

Description

This PR adds interpolation to 1D data operations in SasView. 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.

Fixes # (issue/issues) - I could not find the relevant issue; please comment below when found.

How Has This Been Tested?

Load 1D example data and try different operations. The points used for the operation, including the subset of dataset 1 and the interpolated points of dataset 2 are plotted with a new color when the data operation is computed.

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

@gonzalezma
Copy link
Contributor

I think this addresses issue #626. It was a closed issue, but probably this extends the functionality by allowing larger variations in q?

@butlerpd
Copy link
Member

Interesting .... Was that fixed in 4.x and not ported to 5.x? As far as I know it is NOT fixed in any way in 5.x? people have had difficulty doing simple math because the data were not exactly the same? Or am I wrong?

@caitwolf
Copy link
Contributor Author

Interesting .... Was that fixed in 4.x and not ported to 5.x? As far as I know it is NOT fixed in any way in 5.x? people have had difficulty doing simple math because the data were not exactly the same? Or am I wrong?

After further investigation of #626, the fix only allows for slight variation between q points in the two datasets. You still needed q values of the same length and each pair of q values from dataset 1 and dataset 2 had to be within that tolerance value.

The fix in this PR allows for datasets with different sizes of q values and will perform an interpolation if a q-value in dataset 1 doesn't find a close match within the specified tolerance in q values of dataset 2.

@caitwolf
Copy link
Contributor Author

ci.yml in .github/workflows has been updated to use the paired branch from the sasdata pull request

@caitwolf caitwolf marked this pull request as ready for review January 22, 2024 23:49
Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

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

Minor issues raised.
One functional issue found.

src/sas/qtgui/Calculators/DataOperationUtilityPanel.py Outdated Show resolved Hide resolved
src/sas/qtgui/Calculators/DataOperationUtilityPanel.py Outdated Show resolved Hide resolved
src/sas/qtgui/Calculators/DataOperationUtilityPanel.py Outdated Show resolved Hide resolved
src/sas/qtgui/Plotting/PlotterData.py Outdated Show resolved Hide resolved
@butlerpd
Copy link
Member

Questions: should this go into 6.0.0 beta or be left for 6.1? Also ... this kinda smells like something that should be in sasdata? but that would clearly be another PR?

@caitwolf
Copy link
Contributor Author

Questions: should this go into 6.0.0 beta or be left for 6.1? Also ... this kinda smells like something that should be in sasdata? but that would clearly be another PR?

I realized I never provided a link for the paired pull request in sasdata: SasView/sasdata#62, but I did update the installer so that it was using the paired sasdata changes when testing this branch.

I could go either way on 6.0 vs. 6.1. I need make some changes as a couple tests are failing after responding to the last review. A more thorough update of the documentation should happen along with this feature, especially for the interpolation component. Depending on the timeline of 6.0 I may lean closer to 6.1

@butlerpd butlerpd self-requested a review February 12, 2024 21:53
@butlerpd butlerpd added Discuss At The Call Issues to be discussed at the fortnightly call and removed Discuss At The Call Issues to be discussed at the fortnightly call labels Feb 13, 2024
butlerpd

This comment was marked as duplicate.

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

I have yet to review the code in this section but have done a functionality test using windows 10. I have several observations, some of which may not be for this PR but others I'm sure are?

  • Using the core contrast AOT test data and the apoferritin test data reveals a problem. The low Q data of both data sets is not considered. It is true that the apoferritin data extends beyond the AOT data both at low and high Q but there is still a significant region of overlap that is ignored. I suspect that in fact the data is not being interpolated. At high Q the points are dense enough that I believe they are within tolerance - or many are. It is actually hard to visualize because I cannot zoom in due to the zoom error (plot context menu is broken in 6.0.0a1 #2668) which is still not fixed in main from which this was branched. Clearly this needs to be fixed?
    • NOTE: this does not seem to be a problem for sesans data
  • Loading a 1D or sesans data sets throws the following error
Traceback (most recent call last): File "sas\qtgui\Calculators\DataOperationUtilityPanel.py", line 239, in onSelectData1 File "sas\qtgui\Calculators\DataOperationUtilityPanel.py", line 528, in updatePlot File "sas\qtgui\Calculators\DataOperationUtilityPanel.py", line 442, in operationData1D AttributeError: 'NoneType' object has no attribute 'x'
  • Trying to add sesans and sans1D data returns the error
22:58:10 - ERROR: Unable to perform operation: x ranges of two datasets do not overlap.

That is correct in most cases but for SAXS data at high Q that might not always be true? What happens then? should Data on two different x scales (z vs 1/A) be able to be added? 1/nm vs 1/A should still be doable IF put on the same scale but sesans and not sesans?

  • Help file needs to be updated. It could have used some work before but this change definitely needs to be added.
  • I am unclear on the purpose of the reset button other than clearing the graphs in the data operations panel. Some quirks noted that are probably pre-this PR in which case they should be documented in issues:
    • Changing the operation (+ to * for example) resets the output data panel. However changing the input data (either 1 or 2) does not. This seems inconsistent behavior.
    • Once saved the new data shows up in data explorer and in the drop downs in the data operations panel. However loading in new data to the explorer does NOT show up in the drop down. Furthermore, clicking the reset button does not help either (so what is reset for?).
    • Finally, removing data from the data explorer with or without resetting the data operations panel does not remove the data. No error is thrown however when choosing those deleted files as they seem to still be somewhere?. Closing and re-opening the panel does however clear the now deleted files from the dropdowns.

@caitwolf caitwolf force-pushed the interpolation_for_operations branch from 585c81c to 74c455a Compare March 25, 2024 18:16
@caitwolf caitwolf changed the base branch from main to release_6.0.0 March 25, 2024 18:18
@caitwolf caitwolf marked this pull request as draft April 9, 2024 13:11
@krzywon krzywon changed the base branch from release_6.0.0 to main October 25, 2024 14:47
@wpotrzebowski
Copy link
Contributor

It seems that SasView/sasdata#62 needs to be merged first?

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.

5 participants