CIRDLES / Squid

Squid3 is being developed by the Cyber Infrastructure Research and Development Lab for the Earth Sciences (CIRDLES.org) at the College of Charleston, Charleston, SC and Geoscience Australia as a re-implementation in Java of Ken Ludwig's Squid 2.5. - please contribute your expertise!
http://cirdles.org/projects/squid/
Apache License 2.0
12 stars 27 forks source link

Question - this value model self-references, but still healthy. Is that normal? #516

Closed NicoleRayner closed 3 years ago

NicoleRayner commented 3 years ago

This is maybe a better question for @sbodorkos rather than an issue, but here goes anyway. I recently noticed that one of my expressions in the demonstration squid project gave a nonsensical result. I eventually realized it was because the expression itself (Hfsens) was referred to in the error portion of the value model expression (see screen shot below). So technically the expression was healthy because it had all the arguments it needed, but it referred to itself. Is there any situation where this could be true and still lead to a correct result? Or should this also be a flag that results in a an expression being classified as "unhealthy".

circular reference in value model

I have since fixed this all up and will be providing an updated, error free demo file shortly.

sbodorkos commented 3 years ago

@NicoleRayner I thinkin the absolute strictest arithmetical sense, it's fine, because of the missing 'default' array-indices. The yellow Hfsens on the left refers to the 3-component ValueModel array arising from the expression (value, uncertainty, Boolean), whereas the Hfsens that is part of the expression (on the right) is (I think) actually a 'default' reference to Hfsens[0], i.e. the Value part of the ValueModel array.

So strictly, the expressions here should be written (and, in my opinion, portrayed) as:

Hfsens = ValueModel( Hfsens[0], Hfsens[1], Hfsens[2] )

where Hfsens[0] = 8200/ Hf_bwt[0] and Hfsens[1] = Hf_bwt[1] / Hf_bwt[0] * Hfsens[0]

The second equation can be rearranged as:

Hfsens[1] = 8200 * Hf_bwt[1] / ( Hf_bwt[0] ) ^2

Arithmetically, that's fine. Isotopically, I don't understand it, but that's a different issue...

At the moment the array-index [0] is optional in expressions i.e. if you write Hf_bwt, the Expression Manager assumes you mean Hf_bwt[0]. SQUID 2.50's Task Editor behaved similarly. I think that convention was OK in SQUID 2.50, where it was usual to treat expressions generating a value and its uncertainty separately. I'm not sure it's such good practice in Squid3, where the ValueModel binds value and uncertainty more tightly together.

Plus people (sensu Squid3 users) don't tend to naturally think in terms of zero-addressed arrays, not least because the user-facing sections of SQUID 2.50 did not use them. Even people who do understand the use of array-indexing in the Expression Manager will need to remember that Hf_bwt = Hf_bwt[1] in SQUID 2.50, but Hf_bwt = Hf_bwt[0] here.

I would prefer to see all arithmetic involving array-elements explicitly labelled with the index, as per my rewritten equations above. But I am not sure whether the broader Squid3 user-base would expect (or enjoy) that. It might be cumbersome if people are mostly writing expressions to handle Values only... but it would reduce the sort of confusion that is the basis of this issue.

And it is definitely confusing! I had to look at it for ages, because I was convinced it was not healthy/normal, but I couldn't understand why Squid3 would not have crashed out...

bowring commented 3 years ago

Getting to the essence: a ValueModel does not do any calculation itself, rather it stores a value expression and the uncertainty expression for that value expression. Thus the reference to Hfsens[0] in the uncertainty part of the expression is really 8200 / Hf_bwt[0] as @sbodorkos shows. The preferred usage would be to use the latter.