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

EnginXCalibrateFull: extend bank calibration output table, write it to file, apply calibration on workspace #12957

Conversation

FedeMPouzols
Copy link
Contributor

This fixes #12609.
This also fixes #12700.

To test:

  • unit/doc/system tests
  • code review
  • manual checks to see that this is consistent (explained below).

The unit and doc test example files are not very nice to test this as they're not full workspaces. You can see how this would work in practice using the file from the system test (it can take a while):

ws = Load(Filename = 'ENGINX00193749.nxs')
# not saving file
# positions = EnginXCalibrateFull(Workspace=ws, Bank=1, ExpectedPeaks = '1.3529, 1.6316, 1.9132')
# saving file
positions = EnginXCalibrateFull(Workspace=ws, Bank=1, ExpectedPeaks = '1.3529, 1.6316, 1.9132', OutputDetectorPositionsFilename='det_pos.csv')

This saves a plain text csv file with all the information that EnginX scientists usually like to inspect. EnginXCalibrteFull also applies the calibration on the workspace (property renamed: InputWorkspace => Workspace).

If you open the instrument viewer, after calibration it should be very visible that the detectors of one bank have been scattered. Probably this is not a very reliable calibration! And if you then run EnginXCalibrateFull on the other bank you should see both scattered:

positions_bank2 = EnginXCalibrateFull(InputWorkspace=ws, Bank=2, ExpectedPeaks = '1.3529, 1.6316, 1.9132', OutputDetectorPositionsFilename='det_pos2.csv')

Also, if you open the output csv file in notepad/excel, etc. it should look fine. An open question is how many digits should we use / do EnginX scientists need?

@Anders-Markvardsen: I've expanded the documentation a bit but I'd say that in general the EnginX algorithms documentation needs more improvements. Please see if it reads well/acceptable for now.

So this is now saving the simple text / csv file that scientists like to have in principle. Having this option doesn't do any harm, but I'd tend to say that we shouldn't use this format if possible for the more automated/GUI code. So I'd create another issue to add other format(s) (nexus processed, or parameters xml, or the new calibration nexus, etc.), that would be used from the GUI, and maybe complemented with a button that would produce/show a mantid table with all the old/new positions in x-y-z and spherical coordinates.

@FedeMPouzols FedeMPouzols added the Diffraction Issues and pull requests related to diffraction label Jun 26, 2015
@FedeMPouzols FedeMPouzols added this to the Release 3.5 milestone Jun 26, 2015
@Anders-Markvardsen
Copy link
Member

When I run

ws = Load(Filename = 'ENGINX00213855.nxs')
positions = EnginXCalibrateFull(Workspace=ws, Bank=1, ExpectedPeaks = '1.3529, 1.6316, 1.9132', OutputDetectorPositionsFilename='det_pos.csv')

I get the red results log messages:

Error in execution of algorithm EnginXFitPeaks:
Could not fit any peak. Please check the list of expected peaks, as it does not seem to be appropriate for the workspace given. More details: Could find 3 peaks using the algorithm FindPeaks but then it was not possible to fit any peak starting from these peaks found and using 'BackToBackExponential' as peak function.
  at line 78 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXFitPeaks.py'
  caused by line 187 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXFitPeaks.py'
Error in execution of algorithm EnginXCalibrateFull:
Could not fit any peak. Please check the list of expected peaks, as it does not seem to be appropriate for the workspace given. More details: Could find 3 peaks using the algorithm FindPeaks but then it was not possible to fit any peak starting from these peaks found and using 'BackToBackExponential' as peak function.
  at line 78 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXFitPeaks.py'
  caused by line 187 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXFitPeaks.py'
  at line 57 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXCalibrateFull.py'
  caused by line 102 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXCalibrateFull.py'
  caused by line 132 in 'C:/Backup/Backup_folder1/work/code/Mantid/git/mantid/Code/Mantid/Framework/PythonInterface/plugins/algorithms\EnginXCalibrateFull.py'

I would be better if this was e.g. instead:

Error in execution of algorithm EnginXCalibrateFull:
Could not fit any peak. Please check the list of expected peaks, as it does not seem to be appropriate for the workspace given. More details: Could find 3 peaks using the algorithm FindPeaks but then it was not possible to fit any peak starting from these peaks found and using 'BackToBackExponential' as peak function.

@FedeMPouzols
Copy link
Contributor Author

I'm going to change the names of a couple of options as we just chatted about. Those exceptions are now warning/error messages (in another branch that follows on this: #11746).

Anders-Markvardsen added a commit that referenced this pull request Jun 29, 2015
…_calibration_output_table_and_file

EnginXCalibrateFull: extend bank calibration output table, write it to file, apply calibration on workspace
@Anders-Markvardsen Anders-Markvardsen merged commit 011f68d into master Jun 29, 2015
@Anders-Markvardsen
Copy link
Member

Tested included with instrument view and good updated of alg documentation.

About the question: "An open question is how many digits should we use / do EnginX scientists need?" then when the the scientists starts to use this alg in anger perhaps only at that point will instrument scientists be able to come back with an answer for this

@Anders-Markvardsen Anders-Markvardsen deleted the 12609_enginx_extend_bank_calibration_output_table_and_file branch June 29, 2015 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Diffraction Issues and pull requests related to diffraction
Projects
None yet
2 participants