-
Notifications
You must be signed in to change notification settings - Fork 128
AlignAndFocusPowderSlim prototype #38279
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
AlignAndFocusPowderSlim prototype #38279
Conversation
I won't re-run tests on this on as it genuinely fails |
540c68d
to
4394803
Compare
4394803
to
d523b88
Compare
4e13c5c
to
57a16ad
Compare
dec7fda
to
eef6346
Compare
c94d1da
to
8b01ea8
Compare
👋 Hi, @peterfpeterson, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
8b01ea8
to
4b1db75
Compare
👋 Hi, @peterfpeterson, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
4b1db75
to
9a08782
Compare
9a08782
to
93a2eb1
Compare
👋 Hi, @peterfpeterson, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
824cd74
to
1613e00
Compare
1613e00
to
403a1ac
Compare
👋 Hi, @peterfpeterson, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
403a1ac
to
c6bc28b
Compare
55ea2d1
to
2801046
Compare
9ad65b0
to
d406875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is here that is needed for the algorithm. It is a new algorithm entering bench testing, so very low risk approving this.
This is a reimplementation of AlignAndFocusPowderFromFiles that is (currently) hard coded for the VULCAN instrument. It is a bit better than a prototype that we are bundling into mantid so it can get into instrument scientist hands for testing and feedback.
Eventually (maybe even this development cycle), this will support more instruments and functionality of AlignAndFocusPowderFromFiles.
The careful reviewer will notice that the underlying code has an amount of functionality that is not yet exposed to the user (e.g. event filtering). Please don't request it to be exposed/removed here, but it is still valid to request changes to it.
There is no associated issue.
Further detail of work
To test:
Previous call with AlignAndFocusPowderFromFiles versus AlignAndFocusPowderSlim (with the 1GB unit test file)
where old takes (on my laptop) 2.8s (1.7s is for load) and new takes 1.5s.
For VULCAN_217967 (18GB) the difference is a bit larger
where old is killed by oomkiller during load and new takes 16.2s.
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeepers
will take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.