Refraction index None results in type ValidationError

[saeranv]

For the glass sample json:

honeybee-schema/samples/modifier/modifier_glass_generic_exterior_window.json

Lines 2 to 12 in c1a894c

	"modifier": {
	"type": "void"
	},
	"type": "glass",
	"identifier": "generic_exterior_window_vis_0.64",
	"r_transmissivity": 0.6975761815384331,
	"g_transmissivity": 0.6975761815384331,
	"b_transmissivity": 0.6975761815384331,
	"refraction_index": null,
	"dependencies": []
	}

I currently have the Field set like this for the refraction index:

 refraction_index: float = Field(
        default=1.52,
        ge=0,
        le=1,
        description='A value between 0 and 1 for the index of refraction '
                    '(default: 1.52).'
    )

Which is resulting in a ValidationError, for the refraction index set to null, since 'none is not an allowed value'.

So it looks like the correct behaviour to define None/null conditions is to eliminate the property from the json completely, and then pydantic will automatically set the default value (in this case 1.52) when parsing the object.

Let me know if my interpretation is correct, I can fix it in the PR.

[saeranv]

Update: this is also effecting "modifier_glass_air_boundary.json".

[chriswmackey]

Huh. Where did this default value of 1.52 come from, then? The only reason why that happens right now is because @mostaphaRoudsari sets it to None in the __init__ of the Glass modifier class:
https://github.com/ladybug-tools/honeybee-radiance/blob/master/honeybee_radiance/modifier/material/glass.py#L57

Maybe we should change the default value in __init__ to be 1.52 instead of None if that's actually what the default is supposed to be.

[chriswmackey]

Actually, it seems that the use of None right now results in illegal (or at least very implicit) Radiance strings. I am going to change it.

[saeranv]

Re: the '1.52':

It's interesting, in the docstrings the refraction index is said to be a default of 1.52, but the actual default is set as 'None'.

https://github.com/ladybug-tools/honeybee-radiance/blob/6e73b338c4b43135caaa7826d9688d9f7526fddc/honeybee_radiance/modifier/material/glass.py#L25-L66

[chriswmackey]

It looks like it's a default that is built into the Radiance glass modifier. I made a change to honeybee-radiance so that we now explicitly say 1.52 on __init__ and so that we always write a number into the JSON.

Please updated the JSON samples to use 1.52 in the PR you are working on now, @saeranv .

Explicit is better than implicit.

[mostaphaRoudsari]

It looks like it's a default that is built into the Radiance glass modifier. I made a change to honeybee-radiance so that we now explicitly say 1.52 on init and so that we always write a number into the JSON.

This is not desired. I wish you would have waited for me a couple of hours before making the change and trusted that I might have had a reason for keeping the default to None.

The refraction index is an option input in Radiance. This means that both of these definitions are valid in radiance:

void glass test_glass
0
0
3 0.65 0.65 0.65

void glass test_glass
0
0
4 0.65 0.65 0.65 1.52

You should be able to handle it by making the refraction_index optional in the Pydantic schema.

 refraction_index: Optional[float] = Field(
        default=1.52,
        ge=0,
        description='A positive value for the index of refraction '
                    '(default: 1.52).'
    )

Also the description and the le value was incorrect. The value can be larger than 1.

[chriswmackey]

A few things to note:

1. I understand that both radiance strings are acceptable and we obviously need to support the parsing of both. The change that I had merged supports both.
2. The Pydantic code you have there will not work with honeybee-radiance as it exists before my changes. You would have to use default=None for None to be an acceptable input in the schema. And making None a default input implies that the refraction_index does not exist at all, which is not true. Altogether, it's just more implicit than how we have dealt with default numbers elsewhere in the schema like those for rgb transmittance.

[chriswmackey]

One other thing to note about point 2. there is that @MingboPeng is going to get a headache on the honeybee-schema-dotnet side if None is an acceptable input for a float slot.

[mostaphaRoudsari]

Did you notice the use of Optional in my code? You can see a full-fledged example here.

There is no need for adding None as an option. Also you are confusing Pydantic model with OpenAPI specification. There will be no issues on OpenAPI and dotnet side if we use Optional.

Here is a simple example that will hopefully clarify the differences. You should be able to run it as is.

from typing import Optional
from pydantic import Field, constr, BaseModel


class Glass(BaseModel):

    type: constr(regex='^glass$') = 'glass'

    transmittance: float = Field(
        default=0.0,
        ge=0,
        le=1
    )

    refraction_index: Optional[float] = Field(
        default=1.52,
        ge=0,
        description='A positive value for the index of refraction '
                    '(default: 1.52).'
    )

if __name__ == '__main__':
    from pprint import pprint
    glass_1 = {
     "type": "glass",
     "transmittance": 0.5,
     "refraction_index": 1.4
    }

    glass_2 = {
     "type": "glass",
     "transmittance": 0.5,
     "refraction_index": None
    }

    # will work fine
    g1 = Glass.parse_obj(glass_1)
    # will also work fine
    g2 = Glass.parse_obj(glass_2)

    # see below. There is no None in the default values.
    pprint(Glass.schema())

{
    'properties': {
                'refraction_index': {'default': 1.52,
                                     'description': 'A positive value for the '
                                                    'index of refraction '
                                                    '(default: 1.52).',
                                     'minimum': 0,
                                     'title': 'Refraction Index',
                                     'type': 'number'},
                'transmittance': {'default': 0.0,
                                  'maximum': 1,
                                  'minimum': 0,
                                  'title': 'Transmittance',
                                  'type': 'number'},
                'type': {'default': 'glass',
                         'pattern': '^glass$',
                         'title': 'Type',
                         'type': 'string'}},
 'title': 'Glass',
 'type': 'object'}

[chriswmackey]

I didn't realize that. So, if we use Optional, this means that None is only acceptable on the Python end (or any other language that's not strongly types) but @MingboPeng cannot use None on the dotnet end (it just shows up as 1.52 for him)? I guess that's ok, then.

[MingboPeng]

It is good to know we can use Optional to wrap the Union. Previously there are places like boundary condition with Union type, and cannot set Outdoors as the default value, which ended up being an issue when de-serialize string "boundary_condition": null back to schema object.

Now I guess we can add all places with Union like this if it is not required.

[chriswmackey]

Ok. I changed the PR I was sending to honeybee-radiance to just elaborate the docstring so it's clear that this is a case where None implicitly means 1.52. If @saeranv can change the honeybee-schema PR to use Optional, then we are all set here.

@MingboPeng , if you want to open a separate issue for wrapping all Unions with a default of None in the Optional tag, then we can get to it at some point.

[mostaphaRoudsari]

If @MingboPeng can provide a list of items that are facing this issue or a pattern to find them it should be an easy fix.

I'm also thinking that we might want to start writing tests to catch cases that are valid in Pydantic in general but are not desired for our OpenAPI schema.

[chriswmackey]

Given that the PR regarding refraction_index is merged, I think we can close this. Wrapping unions in the Optional tag should happen in a separate issue.