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

Expose 'ExperimentInfo::populateInstrumentParameters' to the Python api. #38684

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ekapadi
Copy link
Contributor

@ekapadi ekapadi commented Jan 23, 2025

Description of work

Although AddSampleLog has been updated to include an UpdateInstrumentParameters flag, this usage is somewhat obscure, and leaves much to be desired in its application.
Simply exposing ExperimentInfo::populateInstrumentParameters will allow the instrument parameters to be updated when required, and will facilitate directly transferring the logs as properties, which promises to be much less error prone.

Summary of work

Add a definition for populateInstrumentParameters to the export specification in Framework/PythonInterface/mantid/api/src/Exports/ExperimentInfo.cpp

Purpose of work

The SNAP instrument is a parameterized instrument, and the positions of its detector banks are indicated using PV-log values.
In the SNAPRed application, these PV-log values are used to calculate the instrument state-ID SHA.  It's important both that these values be transferable in a straightforward manner, but also that when the logs are transferred, the detector-bank positions are correctly updated.

EWM #9157

To test:

A test script "zip" is attached below, and should run properly in the workbench. This script allows the verification both that ExperimentInfo::populateInstrumentParameters is exposed to the Python api, and also that it executes as expected.
Comments about testing:

  1. Since populateInstrumentParameters itself has unit tests elsewhere, there are no new unit tests added for this current PR.
  2. As fakeSNAP.xml will be added to "external_data" in another PR, I didn't want to add it for to this one as well -- that's why it occurs in the script as an inline string.

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

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

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.

@ekapadi ekapadi added Framework Issues and pull requests related to components in the Framework ORNL Team Issues and pull requests managed by the ORNL development team labels Jan 23, 2025
@ekapadi ekapadi added this to the Release 6.12 milestone Jan 23, 2025
@sf1919 sf1919 modified the milestones: Release 6.12, Release 6.13 Jan 24, 2025
@sf1919
Copy link
Contributor

sf1919 commented Jan 24, 2025

As this is pointing at main it should have a v6.13 milestone. The code freeze for 6.12 was over a week ago.

@ekapadi
Copy link
Contributor Author

ekapadi commented Jan 29, 2025

Attaching a "workbench" test script for this PR.
populateInstrumentParameters_PR_check.zip

@ekapadi
Copy link
Contributor Author

ekapadi commented Jan 29, 2025

I'm thinking that my "release note" is in the wrong place. Hopefully one of the reviewers or the gatekeeper can tell me where it's supposed to go? -- Thanks!

@sf1919
Copy link
Contributor

sf1919 commented Jan 29, 2025

The release note should go in the equivalent v6.13 folder

docs/source/release/v6.13.0/Framework/Python/New_features/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework ORNL Team Issues and pull requests managed by the ORNL development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants