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.

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

Conversation

ehennestad
Copy link
Collaborator

@ehennestad ehennestad commented Nov 27, 2024

Motivation

The IZeroClampSeries has three datasets (capacitance_compensation, bridge_balance, bias_current) where the value should be fixed according to the schema.

This fix ensure the values are set to zero when objects are created, and also issues a "read-only" error if users try the set another property value

How to test the behavior?

types.core.IZeroClampSeries()

The values of the datasets listed above should be 0, instead of empty ([]) as before

types.core.IZeroClampSeries('bias_current', 1)

Should now throw an error

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

@ehennestad
Copy link
Collaborator Author

Thanks to @stephprince for helping figuring this out

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

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

@bendichter bendichter self-requested a review November 28, 2024 01:24
@bendichter
Copy link
Contributor

looks like there are some test issues from the schema update but the changes from this PR look good

@ehennestad ehennestad merged commit 7dfde02 into master Nov 28, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants