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

Rename some HeliumAnalyserEfficiency algorithm parameters #38334

Conversation

yusufjimoh
Copy link
Contributor

@yusufjimoh yusufjimoh commented Oct 31, 2024

Description of work

Rename some HeliumAnalyserEfficiency algorithm parameters for consistency

Summary of work

Fixes #38290

To test:

@yusufjimoh yusufjimoh added the ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS label Oct 31, 2024
@yusufjimoh yusufjimoh added the SANS Issues and pull requests related to SANS label Nov 1, 2024
@yusufjimoh yusufjimoh added this to the Release 6.12 milestone Nov 1, 2024
@yusufjimoh yusufjimoh marked this pull request as ready for review November 1, 2024 12:41
@rbauststfc rbauststfc self-assigned this Nov 1, 2024
Copy link
Contributor

@rbauststfc rbauststfc left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this so far. Great spot on the release note that needed moving from a previous PR. I had a few comments, but this is coming along well.

Copy link
Contributor

@rbauststfc rbauststfc left a comment

Choose a reason for hiding this comment

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

Thanks very much for all of the updates. I'm afraid that there are a couple more constants that needed to be updated for consistency with our naming conventions, so I've added comments for those below. Other than that, everything else is looking good now.

@yusufjimoh yusufjimoh force-pushed the 38290-rename-heliumanalyserefficiency-algorithm-parameters-for-consistency branch from 1a9ff59 to 9d89290 Compare November 11, 2024 17:26
Copy link
Contributor

@rbauststfc rbauststfc left a comment

Choose a reason for hiding this comment

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

Thanks very much for this, all of the code changes are looking good now.

I just did a final search for all instances of the old parameter names and it looks like the algorithm documentation needs to be updated. If you look in the description (https://docs.mantidproject.org/nightly/algorithms/HeliumAnalyserEfficiency-v1.html#description) then there are a couple of references to GasPressureTimesCellLength and GasPressureTimesCellLengthError that need to be updated.

Copy link
Contributor

@rbauststfc rbauststfc left a comment

Choose a reason for hiding this comment

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

Code changes are all looking good and the algorithm documentation has also been updated to use the new constant names. This PR also moves the release notes from a previous PR that incorrectly went into the 6.11 folder into the 6.12 folder, so it's great to have that sorted out here too.

@SilkeSchomann SilkeSchomann self-assigned this Nov 15, 2024
@SilkeSchomann SilkeSchomann merged commit 7b21a39 into main Nov 15, 2024
10 checks passed
@SilkeSchomann SilkeSchomann deleted the 38290-rename-heliumanalyserefficiency-algorithm-parameters-for-consistency branch November 15, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: LSS Issue and pull requests managed by the LSS subteam at ISIS SANS Issues and pull requests related to SANS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename HeliumAnalyserEfficiency algorithm parameters for consistency
3 participants