Skip to content

Simplify memory_gpu field in method database entry #428

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

Merged
merged 11 commits into from
Sep 11, 2024

Conversation

jessicavers
Copy link
Collaborator

@jessicavers jessicavers commented Aug 30, 2024

Fixes #356, #357, #358

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation

Copy link
Collaborator

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few comments 🙂 I'd like the comment about the code comment addressed, but the other one isn't such a big deal, feel free to disregard.

Bigger picture, the test locally are mostly passing but with some failures, due to the axis parameter stuff that existed. This issue has recently been fixed, so can you update this branch with the changes from main please; that should pick up:

  • fixes to IRIS tests CI, so then the IRIS tests run ok
  • fixes to docs CI
  • updates to fix failing tests due to axis issues

Copy link
Collaborator

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

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

Cool, thanks for this! 🙂

IRIS tests CI isn't happy only due to paganin memory hook tests, which is planned to get fixed soon in another PR, so this looks good to merge.

@yousefmoazzam
Copy link
Collaborator

yousefmoazzam commented Sep 11, 2024

Weird, for some reason the task checker CI job is failing, even though all checkboxes have been ticked...

I'm gonna consider that a separate issue, and will potentially look into later 😅

@yousefmoazzam yousefmoazzam merged commit d417e5a into DiamondLightSource:main Sep 11, 2024
2 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants