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

Fixes in invariant prespective #2357

Merged
merged 3 commits into from Nov 8, 2022
Merged

Fixes in invariant prespective #2357

merged 3 commits into from Nov 8, 2022

Conversation

wpotrzebowski
Copy link

@wpotrzebowski wpotrzebowski commented Oct 31, 2022

Description

As described in #2334 two issues related to invariant perspective were identified. The firts issue with read only fields for Qmin and Qmax should be fixed now. However the other problem is more deconvoluted as it is related to the fact that we calculate invariant based on (scale * data) - background, which requires correct setting for scale and background. As it turned out for apoferitin example the values coming from fitting may be over/underestimated and therefore Q* may become negative. Warning has been added to the code suggesting checking backround and scale value. In the future we may consider more inteligent guidance.

I've checked documentation. It seems to be consitent but maybe we should add note about scale/background issue?

Fixes #2334

How Has This Been Tested?

  1. Load data set
  2. Send to Invariant
  3. Check if you can modify Qmin and Qmax
  4. (to check other problem) Play with scale and background (e.g. small scale, high background) and check if you get proper warning.

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)

@wpotrzebowski wpotrzebowski marked this pull request as ready for review October 31, 2022 19:48
@wpotrzebowski wpotrzebowski requested review from smk78 and removed request for butlerpd October 31, 2022 19:49
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.

Code looks fine and addresses the very narrow scope of the PR in that the range boxes are now editable. However they seem to be doing nothing at all -- which may be why they were made not editable in the first place? The range sliders are not visible unless extrapolations are set since the plot disappears unless one turns on extrapolation. However, moving them does not update the values in the range boxes nor does it seem to affect the invariant calculation either.

I would recommend finding out who turned off the editing and/or folks who've been using it like @phgilbert? It may be that it should be merged and new tickets put in but I thought it was working well in 5.0.5 so wonder what has happened?

@wpotrzebowski
Copy link
Author

@butlerpd indeed... Good catch. I see branch of code where the data is set, so I can hopefully fix properly but as suggested I would investigate why the fields where disabled in first place.

@wpotrzebowski
Copy link
Author

wpotrzebowski commented Nov 1, 2022

Here is the commit setting Qmin and Qmax to readonly 8a607a9. I will revert it and will try to address full funcionality in separate PR. Still need to investigate if tests work.

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 7, 2022
@butlerpd butlerpd merged commit e2995c8 into main Nov 8, 2022
@krzywon krzywon mentioned this pull request Nov 8, 2022
@krzywon krzywon deleted the invariant_fixes branch November 8, 2022 14:45
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 1, 2023
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.

Invariant failures
2 participants