BE Polyelectrolyte errors (Trac #1155)

butlerpd commented 5 years ago

Sylvain Prevost sent the following in November 2017 (subsequently lost and refound) regarding problems with this model. He believes there are two problems:

In the python code: concentration is rescaled from mol/dm^3^ to 1/Å^3^ only for monomer concentration, not for salt concentration. Further, polymer concentration is probably a misnomer for monomeric units concentration
In the docs:
- The definition of r,,0,,^2^ is wrong, { b/√(48 π L,,b,,) } should be in the numerator, not the denominator: r,,0,,^2^ = b / [ α √(C,,a,, 48 π L,,b,,) ]
- in the main equation, it should read r,,0,,^4^, not r,,0,,^2^
- in the docs, the conversion of concentration from mol/dm^3^ to 1/Å^3^ should be explicitly indicated before any equation
- for consistency, in the main equation, it should read (q^2^ - 12 h/N,,A,, C,,a,, / b^2^) where N,,A,, is Avogadro's constant (as h is supposedly in Å^3^/mol, and assuming C,,a,, has been converted to 1/Å^3^

Migrated from http://trac.sasview.org/ticket/1155

{
    "status": "closed",
    "changetime": "2018-09-28T17:25:34",
    "_ts": "2018-09-28 17:25:34.264591+00:00",
    "description": "Sylvain Prevost sent the following in November 2017 (subsequently lost and refound) regarding problems with this model.  He believes there are two problems:\n\n* In the python code:\n  concentration is rescaled from mol/dm^3^ to 1/&Aring;^3^ only for monomer concentration, not for salt concentration. Further, polymer concentration is probably a misnomer for monomeric units concentration\n\n* In the docs:\n * The definition of r,,0,,^2^ is wrong, { b/&radic;(48 &pi; L,,b,,) } should be in the numerator, not the denominator:\n  r,,0,,^2^ = b / [ &alpha; &radic;(C,,a,, 48 &pi; L,,b,,) ]\n - in the main equation, it should read r,,0,,^4^, not r,,0,,^2^\n - in the docs, the conversion of concentration from mol/dm^3^ to 1/&Aring;^3^ should be explicitly indicated before any equation\n - for consistency, in the main equation, it should read (q^2^ - 12 h/N,,A,, C,,a,, / b^2^) where N,,A,, is Avogadro's constant (as h is supposedly in &Aring;^3^/mol, and assuming C,,a,, has been converted to 1/&Aring;^3^",
    "reporter": "butler",
    "cc": "",
    "resolution": "fixed",
    "workpackage": "SasView Bug Fixing",
    "time": "2018-08-13T22:15:36",
    "component": "SasView",
    "summary": "BE Polyelectrolyte errors",
    "priority": "blocker",
    "keywords": "",
    "milestone": "SasView 4.2.0",
    "owner": "butler",
    "type": "defect"
}

butlerpd commented 5 years ago

Trac update at 2018/08/14 00:03:53: butler changed description from:

Sylvain Prevost sent the following in November 2017 (subsequently lost and refound) regarding problems with this model. He believes there are two problems:

In the python code: concentration is rescaled from mol/dm^3^ to 1/A^3^ only for monomer concentration, not for salt concentration. Further, polymer concentration is probably a misnomer for monomeric units concentration

In the docs:

The definition of r,,0,,^2^ is wrong, { b/√(48 pi Lb) } should be in the numerator, not the denominator: r,,0,,^2^ = b / [ α √(Ca 48 pi Lb) ]

in the main equation, it should read r,,0,,^4^, not r,,0,,^2^

in the docs, the conversion of concentration from mol/dm^3^ to 1/A^3^ should be explicitly indicated before any equation

for consistency, in the main equation, it should read (q^2^ - 12 h/NA Ca / b^2^) where NA is Avogadro's constant (as h is supposedly in A^3^/mol, and assuming Ca has been converted to 1/A^3^

to:

Sylvain Prevost sent the following in November 2017 (subsequently lost and refound) regarding problems with this model. He believes there are two problems:

In the python code: concentration is rescaled from mol/dm^3^ to 1/Å^3^ only for monomer concentration, not for salt concentration. Further, polymer concentration is probably a misnomer for monomeric units concentration

In the docs:

The definition of r,,0,,^2^ is wrong, { b/√(48 π L,,b,,) } should be in the numerator, not the denominator: r,,0,,^2^ = b / [ α √(C,,a,, 48 π L,,b,,) ]

in the main equation, it should read r,,0,,^4^, not r,,0,,^2^

in the docs, the conversion of concentration from mol/dm^3^ to 1/Å^3^ should be explicitly indicated before any equation

for consistency, in the main equation, it should read (q^2^ - 12 h/N,,A,, C,,a,, / b^2^) where N,,A,, is Avogadro's constant (as h is supposedly in Å^3^/mol, and assuming C,,a,, has been converted to 1/Å^3^

butlerpd commented 5 years ago

Trac update at 2018/08/14 01:53:37: butler commented:

going through the math (is all in python) it is clear that indeed:

r,,0,,^2^ is calculated as described above - documentation needs to be fixed
The code calculates r_squared**2 which indeed is r,,0,,^4^ - documentation should be fixed
the virial parameter h is supposed to be given in Å^3^ NOT L/mol - equation in docs is ok BUT parameter units needs to be changed
Indeed, units would seem to indicate that the salt concentration should also be converted to 1/Å^3^. Looking at the IGOR code this was originally drawn from, it is interesting to note that indeed Ca and Csa are both calculated ... but then only Ca is used as here. This may be a mistake that goes way back. This needs further checking.

butlerpd commented 5 years ago

Trac update at 2018/08/14 02:13:08:

butler changed owner from "" to "butler"
butler changed status from "new" to "assigned"

butlerpd commented 5 years ago

Trac update at 2018/09/27 14:37:21:

butler commented:

At this point all remaining tickets should either be blockers or critical. Critical would be those that do not stop the actual release but need to be dealt with soon after such as updating the marketplace. Tickets such as this are blockers in that if not fixed the docs need to have an appropriate warning in them for this release. Once that is fixed the ticket moves to 4.3 if it is not closed.

butler changed priority from "major" to "blocker"

sasview-bot commented 5 years ago

Trac update at 2018/09/28 17:25:34:

GitHub noreply@github.com commented:

In changeset a5bcd61ecf04134efa21afe21e10363f33f72063:

#!CommitTicketReference repository="sasmodels" revision="a5bcd61ecf04134efa21afe21e10363f33f72063"
Merge pull request #310 from SasView/ticket-1155BE_PolyElectrolyte

Fix docs and code for be_polyelectrolyte.py

Closes #1195
Will open a new ticket that validation is still required.

GitHub noreply@github.com changed resolution from "" to "fixed"
GitHub noreply@github.com changed status from "assigned" to "closed"

SasView / sasview

BE Polyelectrolyte errors (Trac #1155) #1195