Support for SAXS fitting (v2) #3786
Conversation
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
I haven't tested, but see my code comments.
|
@krzywon No, it is ready for merging. The |
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
One final code review. I'll do a functionality test later.
|
|
||
|
|
||
| def change_is_avg(self): | ||
| def change_computation_type(self): |
There was a problem hiding this comment.
Nitpick: It's often a bad idea to completely replace a method, in case there are direct calls to it that may have been missed.
| def change_computation_type(self): | |
| def change_is_avg(self): | |
| self.change_computation_type() | |
| def change_computation_type(self): |
| self.checkboxPluginModel.setEnabled(self.is_avg) | ||
|
|
||
| # set the type of calculation | ||
| match self.cbOptionsCalc.currentIndex(): |
There was a problem hiding this comment.
Potential simplification:
| match self.cbOptionsCalc.currentIndex(): | |
| self.model.set_computation_type(self.cbOptionsCalc.currentIndex()) | |
| match self.cbOptionsCalc.currentIndex(): | |
| case 0 | 1 | 2: | |
| pass |
|
|
||
| # plugin name base is 'SAXS fit' | ||
| # the actual model name includes a structure tag, e.g. 'SAXS fit (2epe)' | ||
| model_name = "SAXS fit" |
There was a problem hiding this comment.
Suggestion: make this an importable constant from SAXSPluginModelGenerator and use it here and in the model, so if we ever change the plugin base name, we only have to change it in one place.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
How could a user arrive here and what should they do to correct it, if they do?
| match self.type: | ||
| case ComputationType.SANS_2D: | ||
| if not (qy is not None and len(qy) > 0): | ||
| raise ValueError("For SANS_2D computation qy must be None or empty") |
There was a problem hiding this comment.
This message is the opposite of what is being checked.
| :param structure_path: Path to the structure file to be used by the plugin. | ||
| :return: The text of the plugin model. | ||
| """ | ||
|
|
There was a problem hiding this comment.
You could remove the os import if you used pathlib.Path for everything here.
Description
After long discussions on how to support SAXS fitting in SasView in the short term, it was decided to extend the generic scattering calculator with a 'SAXS fitting' option.
When this option is selected, the
Computebutton text is replaced withGenerate plugin model. Loading aNuclear datafile with this option enabled circumvents the usual loading logic, and instead forwards the selected file path toAUSAXS. This serves two purposes:AUSAXSalso supportsmmCIFfiles, which is the new standard in biological scattering, andPDBloading logic with the additional information required for SAXS calculations.Clicking the
Generate plugin modelwill then create a newSAXS fit (<filename>)model, which is automatically selected & opened for the user in the fit panel. The SAXS data may then be loaded in & fitted.Though a little awkward, I found this implementation to work decently well, at least for a short-term solution. In the long run, I'd like to have the following:
AUSAXSwhich are currently difficult/impossible to expose to the user. Though this could also be implemented through a dedicated SAXS fitting window, for (1) to work, this is required.SasViewfile loaders to supportmmCIF& fullPDBdata loading, if we want to avoid relying onAUSAXSfor this. Alternatively, we could continue to rely onAUSAXSto parse the files, but store the data on theSasViewside. Or just keep it as is. I'm indifferent on this issue.This PR replaces #3194.
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation
Do we need documentation for this? There's links to the
AUSAXSpaper in the generated plugin model file itself, if people are interested.Installers
Licensing (untick if necessary)