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

generalise broad peak model #458

Merged
merged 4 commits into from
Mar 1, 2022
Merged

generalise broad peak model #458

merged 4 commits into from
Mar 1, 2022

Conversation

dehoni
Copy link
Contributor

@dehoni dehoni commented May 18, 2021

The model "broad_peak" can be generalized to cover Lorentz like behavior and Debye-Anderson-Brumberger by introducing a outer power term with "exponent_p". The commit does the necessary changes.

@dehoni dehoni requested a review from butlerpd May 18, 2021 12:45
@dehoni dehoni self-assigned this May 18, 2021
@dehoni dehoni linked an issue May 18, 2021 that may be closed by this pull request
@dehoni dehoni added this to the sasmodels Next Release +1 milestone May 18, 2021
@butlerpd
Copy link
Member

butlerpd commented Jun 8, 2021

Will this break backward compatibility with old projects? Does it require some translations for that compatibiltiy?

@krzywon
Copy link
Collaborator

krzywon commented Jul 6, 2021

To keep backward compatibility with old save projects saved with old parameter names you will need to update sasmodels/conversion_table.py. The python file houses a dictionary that maps the previous SasView version to the changes made after that version was released. This can accommodate model name and parameter name change.

General usage, but more information is given in the file documentation:

<old_Sasview_version> : {
<new_model_name> : [
<old_model_name> ,
{
<new_param_name_1> : <old_param_name_1>,
...
<new_param_name_n> : <old_param_name_n>
}
]
}

@krzywon
Copy link
Collaborator

krzywon commented Jul 6, 2021

@dehoni
Copy link
Contributor Author

dehoni commented Jul 26, 2021

Tested to save a project and analysis after fitting with some test data from the PR and load it to the Release version, and vice versa. The parameters are passed on correctly and files are read smoothly.

Copy link
Collaborator

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

The documentation and mathematics in the code match each other, and the conversion table update matches what is expected. The build failure is due to a doc building failure that was fixed in #462. There is an issue in SasView, noted in SasView#1902 when loading projects with reparameterized models. Whether this PR should be held up because of the SasView issue is up for discussion.

@butlerpd
Copy link
Member

@krzywon to look at work required to resolve [Sasview#1902]. @butlerpd will check what happens for users who might encounter this problem

@butlerpd
Copy link
Member

sasmodels ready for testing on Win

@butlerpd
Copy link
Member

Made a fit using broad_peak fitting the test/1d/hSDS_D2O_2p0percent.xml and saved as a project in versions 4.2.2; 5.0.4; 5.0.5a1 main branch build of 13 Oct 2021. Then installed the sasmodels Jenkins PR build of PR #458.

Results

  • As expected the json files did not run through the converter. So the value of any renamed parameters that are different than the default are NOT imported. In other words this break projects using this model created in prior 5.x versions.
  • Loading the svs project from 4.2.2 would not load at all and instead through the following error
19:22:58 - WARNING: ER(...) function ignored. Using radius_effective(mode, ...) instead if it exists.
19:22:58 - ERROR: Error while reading the project file: No module named 'sas.sascalc.dataloader.readers.cansas_reader_HDF5'
19:22:58 - ERROR: Error while converting the project file: local variable 'datasets' referenced before assignment
  • Loading the svs project from 5.0.4 release version works as expected
  • Loading the svs project from the 5.0.5a1 main branch build of 13Oct produces the same error as above

Conclusion:

  • This PR breaks previous 5.x projects but not due to the PR itself but due to a bug in the 5.x not running the converter.
  • loading 4.x projects have become broken since 5.0.4 but prior to this PR. However that means I cannot check that the conversion is being called here. This seems like something that must have been broken in one of @krzywon commits on the dataloaders?
  • Merging requires more discussion ... or getting the loader fixed.

@butlerpd
Copy link
Member

butlerpd commented Mar 1, 2022

I think we agreed that this could be merged to main once the release branch is created? The SasView Issue, SasView #1902, to fix the problem with loading project has already been listed as a blocker.

@butlerpd butlerpd merged commit a40e673 into master Mar 1, 2022
@dehoni dehoni deleted the BroadPeakModel branch August 16, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broad Peak Model (Trac #724)
4 participants