Void modifier isn't exported correctly within global_modifier_set

[MingboPeng]

I am using this exported model to test schema .net: https://raw.githubusercontent.com/ladybug-tools/honeybee-schema/master/samples/model/model_complete_single_zone_office.json

In global_modifier_set, this void modifier shouldn't be existed alone with other modifiers unless I am misunderstanding this modifier:

cc @mostaphaRoudsari

[mostaphaRoudsari]

@MingboPeng, this looks right to me. It's basically equal to this radiance material:

void glass generic_interior_window_vis_0.88
0
0
3 0.958 0.958 0.958

There is no separate void modifier. What would you expect to see?

[MingboPeng]

@mostaphaRoudsari thanks for explaining, I think I found the reason causing issues:

1. Type: is there any specific reason that we use all lowercase here, while the rest objects are all pascal case (glass vs Glass).

2. Optional: adding a default value already makes this field optional, why do we need the Optional key here for refraction_index? This makes refraction_index null instead of 1.52 when it is unset.

[saeranv]

@MingboPeng

1. Type: is there any specific reason that we use all lowercase here, while the rest objects are all pascal case (glass vs Glass).

Actually I believe all the modifiers are lowercase types. You can see this in the samples: https://github.com/ladybug-tools/honeybee-schema/blob/master/samples/modifier/modifier_mirror_invisible.json

2. Optional: adding a default value already makes this field optional, why do we need the Optional key here for refraction_index? This makes refraction_index null instead of 1.52 when it is unset.

I believe only adding a default of None makes it nullable. But in the refraction_index case because we have a non-None default, but we need to have None as an potion, we have defined it explicitly as an Optional.

[MingboPeng]

@saeranv

Actually I believe all the modifiers are lowercase types

Yes, all the modifiers are lowercase, and the rest are not.

I believe only adding a default of None makes it nullable

But make default =1.52 won't make it nullable.

But in the refraction_index case because we have a non-None default, but we need to have None as an potion

So why do we want refraction_index as None while default can be 1.52? What does None mean for refraction_index?

[mostaphaRoudsari]

1. I think the root of the lowercase implementation is how we use them in honeybee-radiance to recreate the modifiers. We can update this if it is causing issues for you @MingboPeng but it takes some work to ensure it doesn't break anything in honeybee-radiance.

2. The complexity here comes from how Radiance glass object works. We had a long discussion over this and I insisted not to put the 1.52 value when is not necessary. You can read more here: (Refraction index None results in type ValidationError #81 (comment)).

We set the field as Optional for the default value to be translated to the OpenAPI schema and C# bindings (#83 (comment)). The intention was to make everything easier for translation but it looks like it is actually making the process harder than easier for you?

[MingboPeng]

@mostaphaRoudsari

1. I would suggest to keep all naming convention constant. One place that is causing issue is in AnyOf type in C#, which I have full control, I cam fix this for AnyOf type if it has to be lowercase. But I don't know if it would cause anywhere else that I don't have control.

2. I think this Optional works well with default 1.52 for non-nullable refraction_index in C#. It is also fine to de-serialize json file that generated from c# client.

I found the problem comes from when C# loads json file that generated from python. In this json, refraction_index can be set to null, which is not valid in C# side.

One solution will be:
In python, when generating json file, don't write out null for optional field, just do not include the refraction_index in this case, so that C# can fill it with default 1.52 instead failed converting null to number.

[MingboPeng]

@mostaphaRoudsari

I insisted not to put the 1.52 value when is not necessary.

I guess this refraction_index will be rarely set in Radiance, because in most common cases, users are using the normal glass. That's why you want not to add 1.52?

If this is true, can we create another class: GlassRI, in which refraction_index has to be set or by default 1.52, it cannot be null.
In normal Glass class, we can remove refraction_index field.

[mostaphaRoudsari]

Re: refraction glass - yes. your understanding is correct. @chriswmackey, how do you feel about removing the refraction index from the dictionary when the value is not set? Do we have a rule for such keys in the Python library?

I can try to change the type from lower-case to upper case in both honeybee-radiance and the schema if @chriswmackey is fine with this change.

[MingboPeng]

@mostaphaRoudsari I have updated the dotnet schema to ignore all nulls from json file when de-serializing. This should be fine fro json file created by python with nulls at where it shouldn't be. But not sure if there is any other cases we want to keep nulls when we make null means something.

But getting back to glass refraction index:
If I read the generated schema openApi file correctly, the Optional isn't doing anything differently than the one without Optional.

If this is the case: refraction_index with null should not be valid based on the specification.

[mostaphaRoudsari]

Optional is effective when we use Pydantic to parse/generate the objects. As you pointed out it doesn't affect the OpenAPI specification. That's why you can set refraction_index to None in Pydantic and it will work fine. I'm realizing that this might not be a good practice since our tests will pass in Pydantic but then will fail in SDKs that are generated from the OpenAPI.

[chriswmackey]

Has this been addressed @MingboPeng and @mostaphaRoudsari ?

Or do we need to go through the shcema and take out all use of Optional, replacing the default with None or using Unions of objects with None to ensure that None is acceptable?

[mostaphaRoudsari]

Everything is working currently as is but we have to go back and fix this at some point.

I'm fine with removing optional as you suggested. It can be source of problems that are not ideal.

In the case of refraction index let's just go with the default value. I'm not sure if we want to make an object for float values. It's not ideal but a helpful compromise.