Hardcoded Class Modifications in Schema Are not Checked for

NeurodataWithoutBorders / matnwb

A Matlab interface for reading and writing NWB files

BSD 2-Clause "Simplified" License

49 stars 32 forks source link

Hardcoded Class Modifications in Schema Are not Checked for #43

Open lawrence-mbf opened 6 years ago

lawrence-mbf commented 6 years ago

Main example being the trials Group under NWBFile.

- doc: 'Data about experimental trials'
    name: trials
    neurodata_type_inc: DynamicTable
    quantity: '?'
    datasets:
    - doc: The start time of each trial
      attributes:
        - name: description
          value: the start time of each trial
          dtype: text
          doc: Value is 'the start time of each trial'
      name: start
      neurodata_type_inc: TableColumn
      dtype: float
    - doc: The end time of each trial
      attributes:
        - name: description
          value: the end time of each trial
          dtype: text
          doc: Value is 'the end time of each trial'
      name: end
      neurodata_type_inc: TableColumn
      dtype: float

Expected behavior should be that NWBFile checks for the class along with any additions to the class itself. At the current moment, only the class validation occurs. This isn't necessarily breaking but validation is weaker, and this allows users to break from schema because matnwb never properly checks for these context-dependent values on export.

bendichter commented 4 years ago

@ln-vidrio thanks for documenting this. Do you have any ideas for what would be the best approach to address this?

lawrence-mbf commented 4 years ago

@bendichter Open to other suggestions but here are my thoughts: 1) All generated classes inherit dynamicprops: https://www.mathworks.com/help/matlab/ref/dynamicprops-class.html Probably the simplest method but would require logic at runtime to determine if a given MATLAB property is an attribute, dataset, group or something else and how it should be converted.

2) Generated classes "accumulate" all possible hardcoded changes and add them to the class implementation as optional parameters. This would cause problems if one schema adds hardcoded restrictions that are overridden by another. Hopefully that's not common but I'm not sure what can be assumed given custom extensions.

yarikoptic commented 1 year ago

a pressure point comment: is there a progress to get this old issue which exhibits itself widely to be addressed?

lawrence-mbf commented 1 year ago

There was one more solution proposed aside from the ones above:

Generate the equivalent of anonymous classes which include these changes (like _TimeSeries or something). This will most likely work for writes but reading will require something more advanced like some kind of pattern matching either in the anonymous class name or elsewhere. Not to mention how ugly the class names will end up being.

Open to suggestions though!

yarikoptic commented 1 year ago

I have no clue in matlab and only superficially in NWB structures etc. This issue seems to be the blocker for others (e.g. #238) and seems to be almost 5 years old -- sounds like a good candidate to storm to fix. @rly @oruebel - may be you have feedback on the ideas on how to solve which @lawrence-mbf presented above within the last 3 years?