simphony / simphony-common

The native implementation of the Simphony cuds objects
BSD 2-Clause "Simplified" License
7 stars 5 forks source link

Use keyword arguments for all the generated classes #341

Open mehdisadeghi opened 7 years ago

stefanoborini commented 7 years ago

This means all "required" entries I guess?

tuopuu commented 7 years ago

We should restrict generated classes to only have arguments corresponding to CUBA keys. This "correspondence" could be defined somehow, for instance with ckey.name.lower() where ckey is any of the CUBA keys. I didn't find any generated class right now that has non-CUBA arguments, i.e. arguments not matching with the above description. I think the arguments should stay like that also in future.

stefanoborini commented 7 years ago

'data' is one that doesn't match.

kitchoi commented 7 years ago

'data' is one that doesn't match.

There were discussions between @mehdisadeghi and I regarding getting rid of data in the __init__: https://github.com/simphony/simphony-metadata/pull/44 Maybe there is already another ongoing thread for the topic.

The advantages for removing data from the __init__ signature:

Please also note that there is an opened issue regarding defining data as a CUBA key: https://github.com/simphony/simphony-metadata/issues/21

stefanoborini commented 7 years ago

Ok, then I think I'll do the following. as it seems that data is basically where everything get stored anyway, any object must have it by default, and it should probably not be a construction parameter.

Course of action:

stefanoborini commented 7 years ago

PR #346 removes the "data" entry. All other keys are already with defaults. Some of them don't, but they are the required ones.

mehdisadeghi commented 7 years ago

Sorry for catching up too late, was away a few days.

As @kitchoi has already mentioned we had some related discussions previously. Removing data from the constructor arguments is perfectly fine. The only problem which remains is presenting data as part of the data structure, since it has been one of two most important discussed properties for any given CUDS object since the beginning of SimPhoNy: id and data. So for the moment I suggest to remove it from the code but leave it to be as is in the metadata. We have to rework metadata specification later, we deal with it then.

mehdisadeghi commented 7 years ago

All other keys are already with defaults.

Not all of them have defaults, for example see TEMPERATURE_RESCALING. Anything without a default must also be default to None.

stefanoborini commented 7 years ago

@mehdisadeghi if you default it to None, when you serialize it to hdf5, there's no concept of None in the pytables. None is not a valid float/int/string. So you must have a default in the expected type.

mehdisadeghi commented 7 years ago

@stefanoborini what if in HDF5 we only store properties which have a valid HDF5 value and skip anything else? This way we don't need to worry about concept of None in HDF5.

stefanoborini commented 7 years ago

If you do that, then when you read it back, you won't know if the correct value is Null (never have been specified) or the empty string (was specified as empty), leading to an ambiguity.

ahashibon commented 7 years ago

See here for an internal discussion : https://publicwiki-01.fraunhofer.de/SimPhoNy-Project/index.php/Optional_arguments_to_classes_and_role_of_the_data_attribute

in short: attributes are CUDS or CUBA, the data should remain, and we should allow adding arbitrary CUDS as additional attributes to the data container directly (these are the optional attributes, not necessary specified in the ontology based metadata, but are application specific).