-
Notifications
You must be signed in to change notification settings - Fork 47
Support for SAXS fitting (v2) #3786
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
base: main
Are you sure you want to change the base?
Conversation
krzywon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested, but see my code comments.
|
@krzywon No, it is ready for merging. The |
krzywon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change_is_avg is a horrible function name, though. It does not at all indicate what it is actually doing. A quick check with grep indicates that the old name is not referenced in the code, so I think it is alright. Or do you prefer I change it back?
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could a user arrive here and what should they do to correct it, if they do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like this? It's not critical that this part succeeds, which is why I wrapped it in a try/catch clause.
logger.warning("Could not bring the newly generated plugin model into focus in the Fitting window. Please report this issue.")
|
I tested today and am getting a warning when trying to create the plugin model. I think this is a Win-specific issue. The path saved in the plugin model uses backslashes causing a unicode error. When I open the file generated in the model editor, I get the following error:
|
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)