Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Make inherited read-only datasets from schema read-only properties in matnwb #640

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Fix: Make inherited read-only datasets from schema read-only properties in matnwb #640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d6dd74
Fix: Make inherited read-only datasets in schema read-only properties…
ehennestad Nov 27, 2024
4ec199a
Merge branch 'master' into fix/izeroclampseries-props-should-be-read-…
ehennestad Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion +file/Attribute.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
else
obj.value = [];
obj.readonly = false;
end
end

if isKey(source, 'dims')
obj.dimnames = source('dims');
Expand Down
19 changes: 19 additions & 0 deletions +file/Dataset.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
dtype;
isConstrainedSet;
required;
value;
readonly; %determines whether value can be changed or not
scalar;
shape;
dimnames;
Expand All @@ -22,12 +24,15 @@
obj.type = '';
obj.dtype = 'any';
obj.required = true;
obj.value = [];
obj.readonly = false;
obj.scalar = true;
obj.definesType = false;

obj.shape = {};
obj.dimnames = {};
obj.attributes = [];


if nargin < 1
return;
Expand All @@ -42,6 +47,20 @@
if isKey(source, nameKey)
obj.name = source(nameKey);
end

% Todo: same as for attribute, should consolidate
valueKey = 'value';
defaultKey = 'default_value';
if isKey(source, defaultKey)
obj.value = source(defaultKey);
obj.readonly = false;
elseif isKey(source, valueKey)
obj.value = source(valueKey);
obj.readonly = true;
else
obj.value = [];
obj.readonly = false;
end

typeKeys = {'neurodata_type_def', 'data_type_def'};
parentKeys = {'neurodata_type_inc', 'data_type_inc'};
Expand Down
12 changes: 9 additions & 3 deletions +file/fillClass.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,22 @@
optional = [optional {propertyName}];
end

if isa(prop, 'file.Attribute')
if isa(prop, 'file.Attribute') || isa(prop, 'file.Dataset')
if prop.readonly
readonly = [readonly {propertyName}];
end

if ~isempty(prop.value)
defaults = [defaults {propertyName}];
if isa(prop, 'file.Attribute')
defaults = [defaults {propertyName}];
else % file.Dataset
if isRequired || all(isPropertyRequired)
defaults = [defaults {propertyName}];
end
end
end

if ~isempty(prop.dependent)
if isa(prop, 'file.Attribute') && ~isempty(prop.dependent)
%extract prefix
parentName = strrep(propertyName, ['_' prop.name], '');
parent = classprops(parentName);
Expand Down
4 changes: 2 additions & 2 deletions +file/fillValidators.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
nm = propnames{i};
prop = props(nm);


if isa(prop, 'file.Attribute') && prop.readonly && ~isempty(prop.value)
if (isa(prop, 'file.Attribute') || isa(prop, 'file.Dataset')) ...
&& prop.readonly && ~isempty(prop.value)
% Need to add a validator for inherited and readonly properties. In
% the superclass these properties might not be read only and due to
% inheritance its not possible to change property attributes
Expand Down
53 changes: 10 additions & 43 deletions +types/+core/IZeroClampSeries.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
methods
function obj = IZeroClampSeries(varargin)
% IZEROCLAMPSERIES Constructor for IZeroClampSeries
varargin = [{'stimulus_description' 'N/A'} varargin];
varargin = [{'bias_current' types.util.correctType(0, 'single') 'bridge_balance' types.util.correctType(0, 'single') 'capacitance_compensation' types.util.correctType(0, 'single') 'stimulus_description' 'N/A'} varargin];
obj = [email protected](varargin{:});


Expand All @@ -33,58 +33,25 @@
%% VALIDATORS

function val = validate_bias_current(obj, val)
val = types.util.checkDtype('bias_current', 'single', val);
if isa(val, 'types.untyped.DataStub')
if 1 == val.ndims
valsz = [val.dims 1];
else
valsz = val.dims;
end
elseif istable(val)
valsz = [height(val) 1];
elseif ischar(val)
valsz = [size(val, 1) 1];
if isequal(val, 0)
val = 0;
else
valsz = size(val);
error('NWB:Type:ReadOnlyProperty', 'Unable to set the ''bias_current'' property of class ''<a href="matlab:doc types.core.IZeroClampSeries">IZeroClampSeries</a>'' because it is read-only.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if "read-only" is quite right. That would imply that it has already been written and is being read, which isn't quite true. How about "because it is a fixed value"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am following the convention In MATLAB where properties that are Constant or has SetAccess = private/protected (i.e., users cannot set the value), are "read-only".

Copy link
Collaborator Author

@ehennestad ehennestad Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with this classdef:

classdef SimpleClass
    % SimpleClass - A class with a Constant property
    
    properties (Constant)
        Name=2
    end
    methods
        % Constructor
        function obj = SimpleClass()
        end
    end
end

You will have this error if you try to set the Name property:

>> s=SimpleClass();
>> s.Name=3
Unable to set the 'Name' property of class 'SimpleClass' because it is read-only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok. I'm not crazy about that language but it's fine if it's consistent with existing MATLAB usage

end
validshapes = {[1]};
types.util.checkDims(valsz, validshapes);
end
function val = validate_bridge_balance(obj, val)
val = types.util.checkDtype('bridge_balance', 'single', val);
if isa(val, 'types.untyped.DataStub')
if 1 == val.ndims
valsz = [val.dims 1];
else
valsz = val.dims;
end
elseif istable(val)
valsz = [height(val) 1];
elseif ischar(val)
valsz = [size(val, 1) 1];
if isequal(val, 0)
val = 0;
else
valsz = size(val);
error('NWB:Type:ReadOnlyProperty', 'Unable to set the ''bridge_balance'' property of class ''<a href="matlab:doc types.core.IZeroClampSeries">IZeroClampSeries</a>'' because it is read-only.')
end
validshapes = {[1]};
types.util.checkDims(valsz, validshapes);
end
function val = validate_capacitance_compensation(obj, val)
val = types.util.checkDtype('capacitance_compensation', 'single', val);
if isa(val, 'types.untyped.DataStub')
if 1 == val.ndims
valsz = [val.dims 1];
else
valsz = val.dims;
end
elseif istable(val)
valsz = [height(val) 1];
elseif ischar(val)
valsz = [size(val, 1) 1];
if isequal(val, 0)
val = 0;
else
valsz = size(val);
error('NWB:Type:ReadOnlyProperty', 'Unable to set the ''capacitance_compensation'' property of class ''<a href="matlab:doc types.core.IZeroClampSeries">IZeroClampSeries</a>'' because it is read-only.')
end
validshapes = {[1]};
types.util.checkDims(valsz, validshapes);
end
function val = validate_stimulus_description(obj, val)
if isequal(val, 'N/A')
Expand Down
Loading