Axis can be number (float or int) or str. CompactAxis can be number

[PaulVanSchayck]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.90%. Comparing base (87127e6) to head (dd6cd46).

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #15   +/-   ##
=======================================
  Coverage   93.90%   93.90%           
=======================================
  Files           9        9           
  Lines         279      279           
=======================================
  Hits          262      262           
  Misses         17       17           

Flag	Coverage Δ
unittests	93.90% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lukas-phaf]

I think Python float is the correct representation of a JSON number, so we don't need int here.

There was originally a problem with Pydantic supporting this Union[ValuesAxis[X], ValuesAxis[Y]], is this fixed now?

It would be good to test mixed cases, where we have an array like 3.14, "red". This should fail.

[PaulVanSchayck]

Without this PR, this snippet will print

print(Axes.model_validate_json("""
{
    "x": {
        "values": [
            1,
            2
        ]
    }
}
"""))

x=ValuesAxis[float](dataType=None, coordinates=None, values=[1.0, 2.0], bounds=None) y=None z=None t=None composite=None

With this PR it will print

x=ValuesAxis[int](dataType=None, coordinates=None, values=[1, 2], bounds=None) y=None z=None t=None composite=None

I don't see why the latter is not preferable. Why should the pristine ints be converted to floats, which may result in weird floating point arithmetic issues down the line? I realise JSON has no strict definition of number types, but Python does.

I've not seen an issue so far with this Union[], but haven't tested yet beyond the test cases in this repository.

I've added a test with mixed case.

[lukas-phaf]

I don't think there will be issues with the floating point representation, as all 32 bits integers can be exactly represented in a double precision floating point number. See for example:
https://stackoverflow.com/a/43656339

Although I agree that values=[1, 2] looks cleaner, the library is trying to represent a JSON format, and in JSON 1 is exactly the same as 1.0.

BTW, it is not that JSON has no strict definition of number, they are explicitly defined to be floating point numbers.

[lukas-phaf]

I do think the problem with the Union types in the past was actually here:
https://github.com/KNMI/covjson-pydantic/blob/master/src/covjson_pydantic/ndarray.py#L22

The ValueAxis like in this PR is probably fine.

[lukas-phaf]

I guess in the current code, if you construct the Axes from Python, and the values are Python int's it will fail, because of the strict validation? While with the proposed PR, this will work?

[lukas-phaf]

Two more interesting text cases are:

• values = [1, 2.1]
• values = [1.1, 2]
  I guess the first one will fail, and the second will work fine.

[lukas-phaf]

This was the issue I had with Pydantic v1:
https://docs.pydantic.dev/1.10/usage/model_config/#smart-union

Hopefully resolved in v2.

[PaulVanSchayck]

Both with and without this PR you can pass integers to a float Axis. Even with strict validation on. So this will work:

> print(Axes(x=ValuesAxis[float](values=[1])))
x=ValuesAxis[float](dataType=None, coordinates=None, values=[1.0], bounds=None) y=None z=None t=None composite=None

This will fail with strict mode on (and work with strict mode off).

print(Axes(x=ValuesAxis[float](values=[1])))

This matches what is described in this table: https://docs.pydantic.dev/latest/concepts/conversion_table/#__tabbed_1_5

However, strict mode for ValuesAxis is turned off at the moment. See Issue #4.

I had tested the mixed cases before. They are interesting indeed. Some magic is happening here:

>print(Axes.model_validate_json('{"x": { "values": [  1, 2.1 ] }}'))
x=ValuesAxis[float](dataType=None, coordinates=None, values=[1.0, 2.1], bounds=None) y=None z=None t=None composite=None
>print(Axes.model_validate_json('{"x": { "values": [  1.1, 2 ] }}'))
x=ValuesAxis[float](dataType=None, coordinates=None, values=[1.1, 2.0], bounds=None) y=None z=None t=None composite=None

I think this magic is the smart_union you linked to. So works as intended/expected.

[lukas-phaf]

I think Python float is the correct representation of a JSON number, so we don't need int here.

[lukas-phaf]

This doesn't look "happy" to me. I don't think the intention of the spec is that numbers can be given as string, but instead that the axis is something like "red", "green", "blue".

[PaulVanSchayck]

Agreed. Change to 'real' strings.