Conversation
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
There was a problem hiding this comment.
One final code review. I'll do a functionality test later.
| 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?
There was a problem hiding this comment.
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:
|
|
@krzywon Windows issue should be fixed now. I changed the structure path to always be stored as a posix path instead. Verified it works on my own Windows system. Can you can take a look at the open review conversations? |
|
|
|
This is getting closer! The code looks fine at this point, but running locally, I am seeing an error. Also, closing the GSC and immediately switching to the fitting perspective is a bit jarring and seems like the GSC crashed. Auto-loading the model is useful, but I don't think closing the tool is ideal. Steps to repeat the error below:
|
|
@krzywon what is the expected outcome when trying to calculate the solution scattering by a crystal? My library is complaining that the PDB file is invalid (and indeed it is: there's no element or residue information, only an atom name which can be misinterpreted). |
|
I have now added support for processing files with unknown residues, but I really do not think we should enable this by default. It needs an explicit toggle so users are aware of what is actually being done with their data, which is currently not possible. Regarding automatically closing the GSC: I tried disabling this, and I found that to be an even worse experience, as there is no feedback that something actually happened. I think that, though it may not be ideal, closing the GSC & opening the fitting panel with the model already selected gives much clearer feedback. And it would be the next natural step to take for the user anyway. |
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)