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

Ewm6534 fix missing parameters in call to raw vanadium correction algo #508

Conversation

darshdinger
Copy link
Contributor

@darshdinger darshdinger commented Nov 25, 2024

Description of work

This update addresses a defect in the Raw Vanadium Correction Algorithm within the SNAPRed application. The defect arose due to missing parameters NumberOfSlices and NumberOfAnnuli in calls to the Mantid algorithms CylinderAbsorption and SphericalAbsorption.

These parameters were initially set to default values (NumberOfSlices=1, NumberOfAnnuli=1), leading to potential inaccuracies in the absorption correction calculations. The default values are inadequate for the precise corrections required in vanadium normalization.

By explicitly setting these parameters to NumberOfSlices=10 and NumberOfAnnuli=10, as in the original prototype, this fix ensures the corrected algorithms provide reliable results for vanadium data correction. These changes align the current implementation with the established prototype.

Explanation of work

This PR was simple in execution.

The fix involves:

  1. Adding the parameters explicitly to the algorithm calls within the code.
  2. Ensuring these values match the original prototype for consistency.

To test

Dev testing

Please insure all pytests pass first. Then, with a clear local state directory, take any run and perform both DiffCal and NormCal on the run and insure all features are functional (specifically within NormCal).

CIS testing

Not needed, but same as above.

Link to EWM item

EWM # 6534

Verification

  • the author has read the EWM story and acceptance critera
  • the reviewer has read the EWM story and acceptance criteria
  • the reviewer certifies the acceptance criteria below reflect the criteria in EWM

Acceptance Criteria

This list is for ease of reference, and does not replace reading the EWM story as part of the review. Verify this list matches the EWM story before reviewing.

  • New parameters are set correctly.
  • SNAPRed is fully functional after changes made.

@darshdinger darshdinger marked this pull request as ready for review November 25, 2024 14:27
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.55%. Comparing base (92a9766) to head (68e2c6e).
Report is 2 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next     #508   +/-   ##
=======================================
  Coverage   96.55%   96.55%           
=======================================
  Files          66       66           
  Lines        4959     4960    +1     
=======================================
+ Hits         4788     4789    +1     
  Misses        171      171           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dlcaballero16 dlcaballero16 left a comment

Choose a reason for hiding this comment

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

Tested and runs as expected

@darshdinger darshdinger merged commit 1f0b521 into next Nov 27, 2024
8 checks passed
@darshdinger darshdinger deleted the ewm6534-fix-missing-parameters-in-call-to-Raw-Vanadium-Correction-Algo branch November 27, 2024 17:04
rboston628 pushed a commit that referenced this pull request Dec 6, 2024
#508)

* Add all changes for testing on analysis.

* Reverting changes for WorkflowImplementer.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Remove this, ended up in the wrong branch

* Update diffcal_masking_script.py

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

2 participants