reasonable value improvements

[Awurama-N]

contributes to #1808

[Awurama-N]

After resetting the branch and introducting one at a time (ConstrReasQDef) first, i realised the main issue is with the type for reasV changing from Maybe Expr to Expr.
Please Is there something else i'm missing?

[Awurama-N]

When i went through the code and changed the class HasReasVal to the type Maybe Expr instead of Expr, it fixed the issue. But this time i started with ConstrainedQDefchunk type which doesn't have the reasV access fucntion.
(But the end goal is to have the type be Expr)

[JacquesCarette]

Sorry for not responding earlier, many other tasks on my plate.

Part of the point is to get rid of those Maybes. HasReasVal should return Expr. The point is that ConstrainedQDef should not have an instance of HasReasVal at all. Now, it might make sense to create a new class, MayHaveReasVal, which would have Maybe.

[JacquesCarette]

This change on its own seems fine. Might be better to fix the comments now.

Do you want this merged now, before this is 'plugged in', or wait until it sees some use in an example?

[Awurama-N]

I'll uncomment the new definition after i have checked all the places with the existing constructors, and do the same for ConstrReasQDef

[Awurama-N]

I have now put two of the new chunk types in. The major error i'm trying to deal with is the Expr/ Maybe Expr issue.
Another question i had is with the constructors, I understand that i'm suppose to search for their use in the examples.
What i want to be sure of is: for examnple, if i'm working with ConstrainedQDef which doesn't have a reasonable value, am i going through the use of the constructors to see which constructor fits that description and then switch the definition of that constructor from ConstrainedChunk to ConstrainedQDef or making the changes and then going through to debug.

I guess the major question is what should my objective be whilst looking at the use of the constructors in the examples?

[JacquesCarette]

The code for the new chunk types looks good to me.

To deal with the Maybe issue, you will have no choice but to

1. change HasReasVal to return an Expr (as a lens)
2. add a MayHaveReasVale to return a Maybe Expr (as a getter only)
3. adjust some of the downstream code accordingly
   This will be slightly tricky in some cases.

About changing the use of constructors: your description is correct. Hmm, I see that the simplest examples are actually for ConstrConcept rather than for ConstrainedChunk. (for example

constrainedNRV' q cs = ConstrConcept (dqdWr q) cs Nothing

would be a prime candidate to change, because of that explicit Nothing).

In this case, one to consider is

cuc i t s u space cs rv = ConstrainedChunk (qw (unitary i t s u space)) cs (Just rv)

which has an explicit Just, and thus should be building some that has a reasonable value.

[JacquesCarette]

So I think your 'next step' would be to change ConstrainedChunk to have an Expr instead of Maybe Expr for _reasV. Then adjust the classes HasReasVal and MayHaveReasVal. Then adjust the constructors to build the most appropriate structure. Then we'll see what breaks downstream.

[Awurama-N]

Thank you.
I'm having a bit of trouble trying to figure this error out though:
instance HasReasVal ConstrReasQDef where reasVal = reasV''
--couldn't match type Expr with ConstrReasQDef -> f ConstrReasQDef

[JacquesCarette]

Seems like one is expecting a Lens while the other is just a function. In one direction, you can use view. The other is impossible, so might be a deeper mismatch.

[Awurama-N]

Since i changed the constructors to match the new chunk types i was going through the examples to change those as well. in my most recent commit i changes nomThick from cuc to cnstrw. And i just wanted to make sure that was right before proceeding and pushing other changes

[JacquesCarette]

It certainly is not wrong. Out of curiosity, why is the change needed? Why is cuc no longer the right constructor? [I want you to spell out the reasons, I am not questioning the decision itself.]

[Awurama-N]

when i was editing the constructor definitions from ConstrainedChunk to the new chunk types:

-- | Creates a constrained unitary chunk from a 'UID', term ('NP'), 'Symbol', unit, 'Space', 'Constraint's, and an 'Expr'.
cuc :: (IsUnit u) => String -> NP -> Symbol -> u
  -> Space -> [Constraint] -> ConstrainedQDef
cuc i t s u space = ConstrainedQDef (qw (unitary i t s u space))

-- | Creates a constrained unitary chunk from a 'UID', term ('NP'), 'Symbol', 'Space', 'Constraint's, and a 'Maybe' 'Expr' (Similar to 'cuc' but no units).
cvc :: String -> NP -> Symbol -> Space -> Expr -> ReasonableValueQDef
cvc i des sym space = RVQD (qw (vc i des sym space))

-- | Creates a new ConstrReasQDef from either a 'ConstrainedChunk', 'ConstrConcept', 'UncertainChunk', or an 'UncertQ'.
cnstrw :: (Quantity c, Constrained c, HasReasVal c, MayHaveUnit c) => c -> ConstrReasQDef  
cnstrw c = CRQD   (qw c) (c ^. constraints) (c ^. reasVal)

for cuc i took out the reasonable value so it fit ConstrainedQDef, for cvc I took out the constraint so it fit ReasonableValueQDef so now cnstrw is the only constructor which is used when theres a constraint and a reasonable value.

And in the unitals file nomThick has both a constraint and a reasonable value. It hadn't raised any problems yet, but i thought it made sense to make the change.

[JacquesCarette]

Great explanation - and yes, it makes perfect sense.

[JacquesCarette]

The changes as they are now are good. Is this ready to go?

[Awurama-N]

Yes it can be merged, I am still going through the constructors to make changes but i can just push them after though

[Awurama-N]

Should i go ahead and change wherever HasReasVal is used? since now its lens is Expr and not Maybe Expr.
I could change it to MayHaveReasVal instead so there are no errorrs of "couldn't match Expr to MAybe Expr"

[JacquesCarette]

Change them to MayHaveResVal, since that will be simplest. We can do a second pass on that later.

There is a conflict, so can you merge master first?

[Awurama-N]

Okay

[Awurama-N]

is it okay if I make MayHaveReasVal a getter and then make changes to wherever in the code it's present to suit that?

[JacquesCarette]

Yes.

[Awurama-N]

UncertainChunk used ConstrainedChunk originally but now that ConstrainedChunk has been split into 3 different chunk types do we want to split UncertainChunk into 3 as well? or do we want to make 3 new constructors.
This issue is what is currently breaking the build.

[JacquesCarette]

Take a look at what kinds of UncertainChunk are used in practice. We'll need to support what is currently in use.

[Awurama-N]

UncertainChunk:

Reasonable value only

Drasil.GlassBr.Unitals:

• plateWidth

Drasil.SWHS.Unitals:

• absTol
• relTol

Both constraint and reasonable value

Drasil.GlassBr.Unitals:

• pbTol
• tNT
• plateLen
• CharWeight

I am a bit unsure about where to place absTol & relTol,

unconstrained :: [UncertainChunk]
unconstrained = [absTol, relTol]

I see they're labeled as unconstrained even though their definitions are in this format:

absTol = uvc "absTol" (nounPhraseSP "absolute tolerance") 
  (sub cA lTol) Real
  [physc $ Bounded (Exc, exactDbl 0) (Exc, exactDbl 1)] 
   (dbl (10.0**(-10))) (uncty 0.01 Nothing)

and physc is RealInterval Expr Expr -> Constraint and it's a smart constructor for a range of physical constraints between 2 expressions. But there's not a constraint list in it's definition like how pbTol has [probConstr] for example.

If they count as being constrained then everywhere UncertainChunk is used in the examples has both reasonable value and constraint.

[JacquesCarette]

Yes physc $ Bounded (Exc, exactDbl 0) (Exc, exactDbl 1) is a constraint. So calling them "unconstrained" is wrong!

If all of them have constraints, that simplifies things a lot, since it means we only need a single UncertainChunk that requires both constraints and uncertainty.

[Awurama-N]

Okay great. Should i create a ticket so calling them unconstrained can be changed?

Also this is the current definition for UncertainChunk:

data UncertainChunk = UCh { _conc :: ConstrReasQDef , _unc' :: Uncertainty }
makeLenses ''UncertainChunk

[JacquesCarette]

Yes on the ticket (but you might want to just do it here). So it looks like the current definition of UncertainChunk doesn't have to change, if I'm reading things correctly.

[Awurama-N]

yes okay, it seems like it already does what it's suppose to do so instead would we need to make more constructors? i'm trying to figure out what the next step should be to fix the error occuring right now

[JacquesCarette]

Put the details of the error (i.e. where it occurs and what's the error) in the issue here. Hopefully @balacij can help, as I'll be offline for the next hour.

[Awurama-N]

Okay.

-- | Smart constructor that can project to an 'UncertainChunk' (also given an 'Uncertainty').
uncrtnChunk :: (Quantity c, Constrained c, MayHaveReasVal c, MayHaveUnit c) => 
  c -> Uncertainty -> UncertainChunk
uncrtnChunk q = UCh (cnstrw q)

-- | UncertQs are conceptual symbolic quantities with constraints and an 'Uncertainty'.
-- Contains a 'ConstrConcept' and an 'Uncertainty'.
data UncertQ = UQ { _coco :: ConstrConcept , _unc'' :: Uncertainty }
makeLenses ''UncertQ

[JacquesCarette]

The error message is quite good: you have MayHaveReasVal as a class constraint, but need HasReasVal instead.

[Awurama-N]

HasReasVal was what was originally there, but because I changed HasReasVal to be just Expr instead of Maybe Expr I thought it was right to change this as well from HasReasVal to MayHaveReasVal since it now does what HasReasVal previosuly did.

So should i just change that back then?

[JacquesCarette]

Let the types tell you what to do. I think you require a reasonable value, so Expr is needed.

[Awurama-N]

I reverted some changes i made since the last push and changed the MayHaveReasVal back to HasReasVal, but it brought back the old error i was originally trying to fix with these changes.

[JacquesCarette]

Perhaps you're missing an instance? That seems to be what the error indicates.

[JacquesCarette]

Perhaps if you could push what you think is something reasonable, I can take a deeper look.

[JacquesCarette]

Thanks, looking now.

[JacquesCarette]

In digging deeper, it seemed that this exact design was destined to fail. A new design will have to be drawn up.