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

EnginXCalibrate: add option to produce table with output parameters #806

Merged

Conversation

FedeMPouzols
Copy link
Contributor

This fixes #11806 Adds a new optional property. If you give a name, a table workspace will be created (with that name) that contains the (for now) two output parameters from EnginXCalibrate.

To test:

  • Tests should pass. The new option is tested in a doc-test but I refrained from adding a test case for it in the unit test as that would further slow it down
  • Code review.
  • See if you like the option added. There would be other alternatives but this one looks simple and clean.
  • You can also double-check if it works locally with commands like:
difc, zero = EnginXCalibrate(Filename="ENGINX00213855.nxs", ExpectedPeaks=[1.097, 2.1], Bank=1, OutputParametersTableName='foo_params_table') 
difc, zero = EnginXCalibrate(Filename="ENGINX00213855.nxs", ExpectedPeaks=[1.097, 2.1], Bank=1)  # no table produced

Relase notes: at the moment I'm not adding anything about these EnginX changes, as this is very much work in progress. I'd wait until we have a cleared view on what will be the more final directions and outcomes, etc. Then we can write more sensible release notes about EnginX.

@AndreiSavici
Copy link
Member

Can you use import mantid.simpleapi instead of from mantid.simpleapi import CreateEmptyTableWorkspace. Your approach yields an extra pylint warning

@FedeMPouzols
Copy link
Contributor Author

@AndreiSavici : I think that the last commit should fix that warning. I wonder why pylint doesn't like CreateEmptyTableWorkspace. What a crumpy tool!

@AndreiSavici
Copy link
Member

@FedeMPouzols: In mantid.kernel or mantid.api we have an __init__ function that takes care of which functions are available. So you can do from mantid.kernel import V3D. Unfortunately mantid.simpleapi does not have this option. Neither has numpy

@@ -26,6 +27,12 @@ def PyInit(self):
Direction.Input, PropertyMode.Optional),\
"Calibrated detector positions. If not specified, default ones are used.")

self.declareProperty('OutputParametersTableName', '', direction=Direction.Input,
doc = 'Name for a table workspace with the calibration parameters calculated by '
'from this algorithm: difc and zero parameters for GSAS. At the moment '
Copy link
Member

Choose a reason for hiding this comment

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

Minor suggestion for change, that is change:

Name for a table workspace with the calibration parameters calculated by '
                             'from this algorithm: difc and zero parameters for GSAS. At the moment '
                             'these two parameters are added as two columns in a single row. If not given, no'
                             'table is generated.

to

Name for a table workspace with the calibration parameters calculated '
                             'from this algorithm: difc and zero parameters for GSAS. '
                             'These two parameters are added as two columns in a single row. If not given, no'
                             'table is generated.

@FedeMPouzols
Copy link
Contributor Author

I just made the suggested change in the last commits. I also did small edits to the messages that come from other algorithms to make them a bit more verbose/understandable.

I found that the messages like

i_min = 50 i_max = 10167
cen=20193.6 fwhm=85.2446 height=6.40651 a0=-17.7074 a0=0.000915899 a2=0 chsq=0.411377
cen=38645.3 fwhm=160.154 height=16.6154 a0=-71.9592 a0=0.00188234 a2=0 chsq=0.175314

come from FindPeaks with 'warning' level. It seems to me that these should not be warnings, but I assume that they're logged cause in some other use cases that information from FindPeaks is useful. So I have downgraded these messages to 'notice' level which shouldn't show in (worrying) orange. I'd even go further and downgrade these to 'information' level which is actually used in other places in FindPeaks.

@FedeMPouzols
Copy link
Contributor Author

Oops, the changes to FindPeaks.cpp produced a merge conflict with master. I'm fixing it...

…te_output_table_with_difc_zero

Conflicts:
	Code/Mantid/Framework/Algorithms/src/FindPeaks.cpp

fix conflict (clang-format and var names), re #10967
@FedeMPouzols
Copy link
Contributor Author

I have changed the log level of the two messages mentioned above to 'notice' and was thinking about going down to 'information', but then @Anders-Markvardsen and @peterfpeterson seem to agree that these two should be 'debug' ideally. I'd also second that, as they show very low level info. This change to 'debug' is for the moment 'blocked by' issue #12686 .

Anders-Markvardsen added a commit that referenced this pull request Jun 15, 2015
…ut_table_with_difc_zero

EnginXCalibrate: add option to produce table with output parameters
@Anders-Markvardsen Anders-Markvardsen merged commit d1a9805 into master Jun 15, 2015
@Anders-Markvardsen Anders-Markvardsen deleted the 10967_enginXcalibrate_output_table_with_difc_zero branch June 15, 2015 14:10
@Anders-Markvardsen
Copy link
Member

output when run is now (which all output same colour):
LoadDetectorsGroupingFile started
LoadDetectorsGroupingFile successful, Duration 0.12 seconds
DeleteWorkspace started (child)
DeleteWorkspace successful, Duration 0.00 seconds
Finding peaks from giving starting point, with interval i_min = 50 i_max = 10167
Peak parameters found: cen=20193.6 fwhm=85.2446 height=6.40651 a0=-17.7074 a1=0.000915899 a2=0 chsq=0.411377
Peak parameters found: cen=38645.3 fwhm=160.154 height=16.6154 a0=-71.9592 a1=0.00188234 a2=0 chsq=0.175314
CreateEmptyTableWorkspace started (child)
CreateEmptyTableWorkspace successful, Duration 0.00 seconds
EnginXCalibrate successful, Duration 4.09 seconds

Further findpeak output will be reduced to debug in 12807

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
Development

Successfully merging this pull request may close these issues.

EnginX Calibrate Output
3 participants