2982: Allow density to be blank in SLD Calculator

[krzywon]

Description

This allows the density value to be blank in the SLD calcuator tool for cases where the density is specified in the formula.

Fixes #2982

How Has This Been Tested?

Tested locally that the result from the issue is the result when the formula and no density are entered.

Review Checklist:

[if using the editor, use [x] in place of [ ] to check a box]

Documentation (check at least one)

• There is nothing that needs documenting
• Documentation changes are in this PR
• There is an issue open for the documentation Need to update SLD documentation #3080

Installers

• There is a chance this will affect the installers, if so
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)

Licencing (untick if necessary)

• The introduced changes comply with SasView license (BSD 3-Clause)

[wpotrzebowski]

I am still getting 2.75 from 50%vol H2O@1 // D2O@1.1

[rozyczko]

Entering 50%vol H2O@1 // D2O@1.1 and removing the default density value results in the correct value being displayed.

However, clearing out the density field before the formula is entered (with only the default H2O) results in an assert:

11:20:06 - sas.qtgui.Perspectives.Fitting.FittingWidget - ERROR - Traceback (most recent call last):
  File "C:\projects\sas\sasview\src\sas\qtgui\Calculators\SldPanel.py", line 194, in recalculateSLD
    results = neutronSldAlgorithm(str(formula), density, float(neutronWavelength))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\projects\sas\sasview\src\sas\qtgui\Calculators\SldPanel.py", line 57, in neutronSldAlgorithm
    neutron_scattering(
  File "C:\Users\piotrrozyczko\.conda\envs\sasview\Lib\site-packages\periodictable\util.py", line 96, in _require_kwds
    return function(*args, **kw)
           ^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\piotrrozyczko\.conda\envs\sasview\Lib\site-packages\periodictable\nsf.py", line 874, in neutron_scattering
    assert compound.density is not None, "scattering calculation needs density"
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: scattering calculation needs density

This worked properly before.

[krzywon]

Check the updates I just added. This should be more robust in error catching before the information is sent for calculation.

[rozyczko]

the code looks good and the fix addressed the original issue.

[wpotrzebowski]

Ok, I didn't read the instructions properly... It works. I am wondering however how users should know about the blanking density field? I guess one option is to leave it blank by default but then we won't be able to start from the example formula. We probably can add warning or at least tooltip?

[krzywon]

I am wondering however how users should know about the blanking density field?

I think a good solution for this is, if the density is specified in the formula, we could disable and gray out the density field and ignore any value in the box. This would give a visual user cue something has happened, while keeping the calculation true to the user intent.

[butlerpd]

Actually, This issue will require some small changes to the documentation. I will open an issue around that.

[butlerpd]

The code changes look reasonable and already approved by @rozyczko. testing fairly thoroughly the parser seems to be doing a good job of robustly managing not only the new changes but the problem of re-computing while typing causing errors when one does not type too fast.

However, I do not believe the changes to the documentation are sufficient. Everything from Specifying Materials or Mixtures in the Molecular Formula Field down to Biomolecules should be read and edited appropriately. This could be done in a separate PR as originally suggested or done in this PR given the docs have already started to be edited in this PR. Assuming the latter, this still needs changes. However if we decouple then this can be merged IMO

[butlerpd]

@krzywon it may be more expedient to open another issue after all and let me do the documentation edits? Up to you.

[krzywon]

it may be more expedient to open another issue after all and let me do the documentation edits? Up to you.

Working on it now in this branch

[krzywon]

@butlerpd - take another look when you get a chance. I have updated the documentation and made another fix related to the formula entry to ensure all formulas in the documentation are valid.

[butlerpd]

OK @krzywon - will work on that next then

[butlerpd]

went deep in a rabbit hole on this. It is much more complicated that I realized and I see why it took @katieweigandt so long to get the docs sorted! I now see why greying out the box may not always be the desired behavior. individual densities are required for all vol% entries in order to get the total scattering length. However non-ideal mixing means it may not always give you the right materials density.

I know there was some discussion about this earlier between @wpotrzebowski and @krzywon which led to replacing the "leave blank" with "grey out." I will admit that sounded good to me too this morning, but now I think that was the wrong answer. As @katieweigandt kept emphasizing in the docs: the best is to actually measure the density of your solution if you can. It is just that this is often not easy to do. Thus I think the right answer would be to go back to the density box overriding but allowing it to be blank without it defaulting to 1.0. That is I think how it works now if we just remove the .setEnabled(False) line (but keep density = None). The error checking is much more robust now so that should work? It may require a bit more editing of the docs? At least making the point that whatever is in the density box will override anything else? There would be the earlier question by @wpotrzebowski about some better way of alerting the novice user I guess?

A couple of other points to note:

• Entering @ , without any numbers following it, defaults to @1.0 - I will argue that is reasonable and not worth trying to capture. If the user stops there I'm not sure what they would expect other than 1?
• Nested expressions may not grey out the density line even if the outer X components all have their densities reported. The density box always supersedes (see example in docs 20%vol (10%wt NaCl // H2O)@1.07 // D2O@1.11) - This should probably be the default behavior anyway if we accept what I said above.

[butlerpd]

This looks good now. Looking at the code change I have not tried to do the full extensive testing again as only entries with multiple components will be affected (containing \\).

This nicely addresses the problem of not allowing for non-ideal mixing and makes the SasView SLD app completely compatible with the online version I think. Moreover the color coded and tool tip on the density override is quite a nice aid for the user. All real errors, which will raise an exception from the periodictables.py all get caught and make the offending box background yellow so not worried about any combination that is not valid.

One confusing point about periodictables.py that maybe should be noted in the documentation is that periodictables.py knows the density of the elements and will assume them if a mixture does not specify its density. But this is not related to this PR so I vote this can be merged and be called done for 6.0.0