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

Problem with RPA model in ESS_GUI 5.x #396

Closed
smk78 opened this issue May 11, 2020 · 8 comments
Closed

Problem with RPA model in ESS_GUI 5.x #396

smk78 opened this issue May 11, 2020 · 8 comments

Comments

@smk78
Copy link
Contributor

smk78 commented May 11, 2020

Not sure whether this is a sasmodels model issue or a sasview QtGUI issue.

Couldn't find an obvious existing issue about this.

Reported by user Greg Smith. Verified by @smk78 .

I can’t make any sense of the RPA model as implemented in SasView 5.0.2. I am attempting to model the scattering from a 1:1 mixture of hydrogenous and deuterated polymers, which I assume to be case 0. I selected this case, and then I found that I was unable to select the level 0 “N” that I assumed corresponded to the binary mixture of homopolymers case. It was not possible to do this, so I entered the values for the two polymers that I was mixing under “N=1”. The calculation does not seem to change compared to the calculation for the default parameters. I succeeded in one attempt to do this in SasView 4 before it started crashing earlier, and the curve did look different then. I have attached a SasView 5 project for this calculation.

RPA_homopolymers_SasView502.zip

To be fair to 5.x, the RPA model in 4.x does not inspire great confidence. The last version where it seems to have worked properly was 3.1.2.

@butlerpd
Copy link
Member

Thanks @smk78 I started looking for an issue on RPA and/or multiplicity models in both sasmodels and sasview repos but found nothing before I ran out of time. I vaguely remember about an issues of the number being ignored and another where the starting value was 1 instead of 0 so that you could never reach 0. For this reason I am guessing it is in the GUI rather than sasmodels?

Note that as I recall RPA is fairly unique in that it is not really a "multiplicity" model but really a case selection. The others (core-multishell etc) use the number to increase the number of parameters by n essentially. So probably there is a difference in behavior on selection?

@butlerpd butlerpd added this to the sasmodels 1.0.6 milestone May 2, 2021
@butlerpd
Copy link
Member

Testing 5.0.5RC2, the RPA is totally broken. I think this is mostly a GUI issue but probably due to changes in how sasmodels handles multiplicity models and control parameters. For example N should not be a dropdown option as the control parameter is the case_num. Clearly this is going to take some significant work but has been problematic in previous versions so am moving this to next release. But this needs to be addressed post haste so will label as blocker.

@smk78
Copy link
Contributor Author

smk78 commented Jan 31, 2022

So should we be putting something in the docs like we have for the paracrystals? Or should we just remove the RPA model from this release? We aren't sure if the paracrystal models are correct, but if the RPA is actually broken, is it right to continue to offer it?
Should anyone need the binary case, I wrote a model for that for a user which is on the Marketplace (http://marketplace.sasview.org/models/124/).

@butlerpd
Copy link
Member

butlerpd commented Feb 1, 2022

The docs are the only thing that can reasonably be achieved prior to a release (this has probably been the case for all of 5.x due to the control parameter issue). Removing the model from sasmodels is not appropriate since the model is perfectly correct I believe and the sasmodels package is used by others than SasView. On the other hand I don't even want to think about the code required for sasview (gui) to be able to ignore just one model in the sasmodels package.
I would argue however that fixing the gui interface to the control parameter, which fixes a number of tickets should be a very high priority for the next release?

@smk78
Copy link
Contributor Author

smk78 commented Feb 15, 2022

Has been decided to keep this issue open for now: #497 (comment)

@butlerpd
Copy link
Member

I just realized that we merged a prominent note in the RPA model code itself (approved my yours truly among others 😄. However, given the comments above, it is not in fact clear that is true. Unless the model itself has changed much, it is probably correct and as a member of the sasmodel package used independently by downstream projects that would be the wrong place to put the notice? On the other hand, if it really is a sascalc/sasgui interface issue it is not clear where/how we deal with that.

That said, one way or another something must be done for users of SasView 5.0.5. We should probably add a sister ticket in the sasview repo as I'm sure at least part of the problem is on the sasview side?

@smk78
Copy link
Contributor Author

smk78 commented Feb 15, 2022

The note in the model description is carefully worded and says that he model is not working correctly in SasView. I was hoping that anyone clever enough to use sasmodels directly would understand the difference...

I'll create the sister ticket.

@butlerpd
Copy link
Member

This is really a GUI issue as mentioned above and should have been an issue in sasview rather than here. Actually the sister issue in sasview, SasView/sasview#2022, was closed when sasmodels was updated (documentation updated) which in hindsight seems completely backwards. Given how old this is (the title talks of ESSGUI), I am closing this and opening anew one in sasview, SasView/sasview#2243, which links to the discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants