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

SasData 66: Ensure realistic resolution values loaded from data #2839

Merged
merged 9 commits into from
Jul 18, 2024

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Apr 1, 2024

Description

This ensures the data resolution is not used when all resolution values loaded from a data set are 0.0. A bug was also fixed where the selected resolution was blank instead of None after selecting a model. In addition, custom pinhole and slit smeared resolutions can be applied to theory data once a model has been selected. This does NOT allow the Hankel transform to be applied to theory data due to differences inhow resolution functions are applied for SESANS data.

Fixes SasView/sasdata#66
Fixes #2095 (does not touch SESANS-specific #2229)

Review Checklist:

Documentation (check at least one)

  • There is nothing that needs documenting
  • Documentation changes are in this PR
    • NOTE: This is all inline documentation to ensure future changes do not change the behavior.
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)

Licencing (untick if necessary)

  • The introduced changes comply with SasView license (BSD 3-Clause)

krzywon added 3 commits April 1, 2024 15:13
…ngle level of checks, and fix logic related to dQ where nothing is greater than 0
…after a model is selected, plus clean up of duplicate code
@butlerpd
Copy link
Member

butlerpd commented Apr 2, 2024

Functionally this seems to fix the issue reported in SasView/sasdata#66. However I don't think it really addresses the #2095 issue since it still requires one to click compute before being able to apply the smearing (as was always the case). However there may be more urgency to fix this based on new conversation in #2095?

…pdate GUI elements as soon as the smearing combo box changes
@butlerpd butlerpd requested a review from caitwolf April 9, 2024 13:39
@butlerpd butlerpd added the SasView 6.0.0 Required for 6.0.0 release label Apr 19, 2024
Copy link
Contributor

@caitwolf caitwolf left a comment

Choose a reason for hiding this comment

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

Revisions look good and the resolution selection operates as described. I tested the Windows installer but could be good to have someone check the Mac installer?

@caitwolf
Copy link
Contributor

Functionally this seems to fix the issue reported in SasView/sasdata#66. However I don't think it really addresses the #2095 issue since it still requires one to click compute before being able to apply the smearing (as was always the case). However there may be more urgency to fix this based on new conversation in #2095?

I noted that the last change does now address #2095. Although I noticed that when smearing is applied to theoretical data, the data will be plotted immediately after the compute button is pressed. It will also briefly flash the theoretical data without smearing before showing the smeared data. When smearing is not applied to theoretical data, the compute button needs to be pressed followed by Compute/Plot in order to plot the data.

@butlerpd
Copy link
Member

Pursuant to @caitwolf review and some more testing with the latest installer on windows, there is indeed a discrepancy in behavior during the initial calculation of newly selected model between doing so with vs without resolution. I would however argue, and I think there is a ticket somewhere asking for this, that the correct behavior should be the one seen when resolution is requested. Thus the change should be to the non-resolution functionality which is not part of this PR. So I thinks we should merge this. On the other hand, I was unable to see the "flash" of the first non smeared calculation plotted as suggested by @caitwolf. That may be because my computer is too fast. Alternatively, if @caitwolf was testing from the development environment maybe it is not present in the windows installer?

That said, doing some more careful testing I note that there are a couple of small subtleties that I suspect should not be part of this PR?

  • Entering a new value for one of the parameter and hitting return acts as if one has hit the Calculate or Compute/Plot button: it will only calculate but not plot the very first time (but the label of the button changes) and will calculate AND plot subsequent times. This is as expected in some sense though highlights the oddity of the initial Calculate only, when all subsequent actions show the new plot immediately.
  • This equivalence is true also when resolution is selected so that in that case entering a value and hitting return (whether that is after entering the resolution value or a new value for a parameter) the calculation and plotting occur together even on the very first enter. Again I would argue this is as it should be.
    • HOWEVER, if the resolution value is entered without hitting return and the calculate button is pressed immediately after, nothing happens at all (the label on the button does not change either) and a second press of the button is required. This seems very odd!!

I also note that the polydispersity tab is active even though the polydispersity checkbox was not checked. However, I believe that was addressed in another PR?

@krzywon
Copy link
Contributor Author

krzywon commented May 23, 2024

if the resolution value is entered without hitting return and the calculate button is pressed immediately after, nothing happens at all (the label on the button does not change either) and a second press of the button is required. This seems very odd!!

I'm seeing this in the installed Windows version, but not every time. A click is required to remove the focus from the dQ/Q box for some reason.

On the other hand, I was unable to see the "flash" of the first non smeared calculation plotted as suggested by @caitwolf.

I see this in the installed version for times where Clicking the Calculate button the first time works.

I also note that the polydispersity tab is active even though the polydispersity checkbox was not checked. However, I believe that was addressed in another PR?

Correct. This branch is a bit behind the release branch. Should I push a merge commit?

@krzywon krzywon merged commit ebe792f into release_6.0.0 Jul 18, 2024
28 checks passed
@krzywon krzywon deleted the sasdata-66-dQ-options branch July 18, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SasView 6.0.0 Required for 6.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants