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

3126: Fix OpenCl and covariance issues #3129

Merged
merged 6 commits into from
Oct 23, 2024

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Oct 17, 2024

Description

Any change to a preference was triggering an OpenCL environment reset, which was resulting in an OpenCL error when rerunning fits. This also fixes the tuple out of range issue noted by @wpotrzebowski in #3122 (comment).

Fixes #3126

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Review Checklist:

Documentation (check at least one)

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • 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)

@wpotrzebowski
Copy link
Contributor

I've tested the Mac installer. The issue I reported earlier seems to be solved but it hangs on building documentation. (I gave up after 30min and forced it to quit).

@wpotrzebowski
Copy link
Contributor

It is strange but model documentation works, however "general help" is hanging forever.
Screenshot 2024-10-21 at 09 47 30

@krzywon
Copy link
Contributor Author

krzywon commented Oct 21, 2024

That shouldn't be triggering at that point. The doc generation should only be active for plugin models. I'm guessing this is on Mac?

@krzywon
Copy link
Contributor Author

krzywon commented Oct 21, 2024

Merging this for now while I investigate the documentation issue

@krzywon
Copy link
Contributor Author

krzywon commented Oct 21, 2024

Merging this for now while I investigate the documentation issue

Never mind - no approvals yet

Copy link
Contributor

@wpotrzebowski wpotrzebowski left a comment

Choose a reason for hiding this comment

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

Tested the functionality, and it works fine. I cannot asses code quickly - sorry.

Copy link
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

So, it's my understanding that you've made it so GPU options go through the same kind of staging process as the other options. This seems sensible.

It looks like this will stop random errors from other preferences, but it will still cause errors if you change the GPU options. If this is acceptable, then it seems fine,

I've added some other changes, mostly stylistic. I would very much like to see the bias one done, and the reshape one would be much neater.

I'm going to approve so things are not held up though.

src/sas/sascalc/fit/BumpsFitting.py Outdated Show resolved Hide resolved
src/sas/sascalc/fit/BumpsFitting.py Show resolved Hide resolved
src/sas/sascalc/fit/BumpsFitting.py Outdated Show resolved Hide resolved
src/sas/sascalc/fit/BumpsFitting.py Outdated Show resolved Hide resolved
@krzywon
Copy link
Contributor Author

krzywon commented Oct 22, 2024

Thanks for the review, @lucas-wilkins. I incorporated most of your suggestions. Have one last look and merge if you think it is ready.

Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

Functionality test of actions/runs/11467760172 on W10/x64.
Tried single parameter fits with/out OpenCl and with/out DREAM. No sign of the errors reported by @wpotrzebowski . LGTM.

@krzywon krzywon merged commit 56a839b into release_6.0.0 Oct 23, 2024
40 checks passed
@krzywon krzywon deleted the 3126-opencl-and-covariance-issues branch October 23, 2024 13:32
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.

4 participants