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

No conversion from <type> to Hl7.Fhir.Model.Quantity #584

Open
baseTwo opened this issue Oct 7, 2024 · 6 comments
Open

No conversion from <type> to Hl7.Fhir.Model.Quantity #584

baseTwo opened this issue Oct 7, 2024 · 6 comments
Assignees
Labels
Milestone

Comments

@baseTwo
Copy link
Collaborator

baseTwo commented Oct 7, 2024

The following types cannot convert to Hl7.Fhir.Model.Quantity:

  • FhirDateTime (Hl7.Fhir.Model namespace?) Issue 362
  • Hl7.Fhir.Model.Integer Issue 373
  • Hl7.Cql.Primitives.CqlQuantity see screenshot in VONK-7083
@baseTwo baseTwo added the Runtime label Oct 7, 2024
@baseTwo baseTwo added this to the NCQA Summit 2024 milestone Oct 7, 2024
@baseTwo baseTwo self-assigned this Oct 7, 2024
@ewoutkramer
Copy link
Member

ewoutkramer commented Oct 7, 2024

See Slack:

No, it's that somewhere along the way, we've started calling Convert when narrowing a choice type into one of its possibilities, which is probably incorrect behavior.
13:43
I updated the As operator to sometimes use ChangeType because at the C# level we have some type problems because our C# type model and the CQL type model aren't 1:1
13:44
But here, we are calling the specific Convert operator which you can actually call for real in CQL source code and I don't see any justification for it
13:45
The intent of the CQL is to check only Observation resources whose value is expressed as a Quantity:
and BMIRatio.value as Quantity is not null
13:45
I would have used an is operator here, but this construct is also valid
13:46
Observation.value[x] is a simple choice type, of which FHIR Quantity is one of them
13:46
We should just be doing a C# as here because Observation.value's runtime value could be a FHIR Quantity. (edited)
13:46
However:
13:47
It is possible that this is happening because we have 2 different model types in play here
13:47
e.g., if Vonk is using a different class for Quantity than the one our TypeResolver is returning for the Firely SDK, then we are gonna have problems
13:48
And we would need to have all of those conversions, but only in Vonk context, meaning we would want Vonk to have custom converters that it adds to the type conversion list because it knows it's not using the exact SDK quantity clases and needs to be able to convert freely between them
13:50
At the time this offending expression was created when converting ELM to C#, we should know that Observation.value is a Choice<FHIR.Quantity, ...> (edited)
13:52
and we know the type in the as CQL expression is FHIR.Quantity. Since we are narrowing a choice type, we should be invoking our .NET As operator. It will be try to use our ChangeType function to perform the change and discover there isn't a conversion, at which point it should fall back to C# as and issue a warning
13:52
I thought that is how I changed the As operator to work, but maybe not

@ewoutkramer
Copy link
Member

The main bit is this:

and BMIRatio.value as Quantity is not null

What the author is doing here is equivalent to an is, but we incorrectly try to find a cast for it, and upon not having one, reporting that we are missing a cast. Instead, we should return null.

A deeper question is: does this as actually try to call implicit conversions? We think not. So you would not call a cast. But then again, we use the casts to allow casts that should be possible because of subtyping, also for the cases where the POCO subtyping is not 1-on-1 with the CQL subtyping. So if we don't call it, we introduce another bug where perfectly valid as statements would not work anymore.

@baseTwo
Copy link
Collaborator Author

baseTwo commented Oct 7, 2024

Start by writing a unit test for
define function test(obs FHIR.Observation): obs.value as Quantity

@ewoutkramer
Copy link
Member

Evan suggested this test:

define f: 1 as System.Long
does not compile
define function test(obs FHIR.Observation): obs.value as Quantity (edited)

@EvanMachusak
Copy link
Collaborator

This CQL:

library AsTest version '1.0.0'

using FHIR version '4.0.1'

define function ObsValue(obs Observation): obs.value as Quantity

Produces this (correct) C#:

using System;
using System.Linq;
using System.Collections.Generic;
using Hl7.Cql.Runtime;
using Hl7.Cql.Primitives;
using Hl7.Cql.Abstractions;
using Hl7.Cql.ValueSets;
using Hl7.Cql.Iso8601;
using System.Reflection;
using Hl7.Cql.Operators;
using Hl7.Fhir.Model;
using Range = Hl7.Fhir.Model.Range;
using Task = Hl7.Fhir.Model.Task;
[System.CodeDom.Compiler.GeneratedCode(".NET Code Generation", "2.0.4.0")]
[CqlLibrary("AsTest", "1.0.0")]
public class AsTest_1_0_0
{


    internal CqlContext context;

    #region Cached values


    #endregion
    public AsTest_1_0_0(CqlContext context)
    {
        this.context = context ?? throw new ArgumentNullException("context");


    }
    [CqlDeclaration("ObsValue")]
	public Quantity ObsValue(Observation obs)
	{
		DataType a_ = obs?.Value;

		return a_ as Quantity;
	}

}

when compiled using our CQL-to-ELM tool.

@baseTwo baseTwo modified the milestones: NCQA Summit 2024, 2024 Q4 Nov 19, 2024
@baseTwo
Copy link
Collaborator Author

baseTwo commented Nov 19, 2024

Will investigate again

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

No branches or pull requests

3 participants