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

Convert API to OOP? #39

Closed
wants to merge 4 commits into from
Closed

Conversation

apjanke
Copy link
Collaborator

@apjanke apjanke commented Jan 1, 2020

Here's a draft example of how you could convert the API and main code of BIDS-MATLAB to Object Oriented Programming (OOP), and maintain back-compatibility with the existing API.

This follows up discussion in #19 starting at #19 (comment).

This only uses basic Matlab OOP features, so it will be compatible with both Matlab, including versions going back several years, and Octave, going back at least to version 4.4.

This approach maintains query() as the main interface for getting data out of a BIDSLayout object, leaving the object largely as an opaque structure. You could break query() down into various sub-query methods if you wanted to; could make tab-completion more useful for users.

This leaves bids.query(), bids.layout(), bids.report(), and bids.validate() as back-compatibility functions with the same effective signatures as before, so existing BIDS-MATLAB client code will continue to work under this change.

Adds a couple utility functions (size2str, mustBeA) that I think are widely useful in Matlab programming, especially OOP.

I think this approach could be a good way to prepare for support of Derivatives and Sources, like PyBIDS supports. The various methods that PyBIDS' own BIDSLayout object supports (documented at https://bids-standard.github.io/pybids/generated/bids.layout.BIDSLayout.html#bids.layout.BIDSLayout) could also be added. Though some of those are supported by the public access to the properties on BIDSLayout that get populated when you read a layout from disk. One approach could be to just mimic the entire API of PyBIDS; Matlab OOP can support that.

Maybe BIDSLayout should go in the sub-package +bids/+layout, the way it's arranged in PyBIDS?

Closes #25. (Package-level private/ dirs don't play well with classes inside the packages, I think.)

This PR's changes are big enough (mostly in terms of moving code around rather than changing it) that you probably want to check out the branch locally and examine it in Matlab, looking at the files in the editor and using "doc" on them to see how their help reads, instead of just reading through the diff for this PR on GitHub.

@apjanke
Copy link
Collaborator Author

apjanke commented Jan 1, 2020

Maybe BIDSLayout should go in the sub-package +bids/+layout, the way it's arranged in PyBIDS?

Yes, it should be done this way. I updated the PR to do so.

@apjanke
Copy link
Collaborator Author

apjanke commented Jan 2, 2020

Tests are failing.

$ octave $OCTFLAGS --eval "results = bids_runtests; assert(all(~[results.Failed]))"
test_bids_examplesXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
* 7t_trt: max_recursion_depth exceeded
* ds000117: max_recursion_depth exceeded
* ds000246: max_recursion_depth exceeded
* ds000247: max_recursion_depth exceeded
* ds000248: max_recursion_depth exceeded
* ds001: max_recursion_depth exceeded
* ds002: max_recursion_depth exceeded

I suspect this is an aliasing issue between the names BIDS and bids which is cropping up only on Linux or Octave. I'll fire up Octave and maybe a Linux VM to track this down.

@apjanke
Copy link
Collaborator Author

apjanke commented Jan 2, 2020

Ah. The issue is that on Octave, functions mask packages with the same names, while on Matlab they do not. (This is because on Octave, you can dot-index into the results of a function call, while on Matlab you cannot, so there's no ambiguity between a subpackage reference and a function call with further indexing.)

The bids.layout function was masking the bids.layout package. To fix this, I renamed the bids.layout function to bids.readlayout. This would break back-compatibility, but it's the only way to have a bids.layout package in line with how PyBIDS is organized.

… package bids.layout on Octave

On Matlab, functions in packages do not mask subpackage names. But in Octave, they do. So we
can't have both a function bids.layout and a subpackage bids.layout.
@Remi-Gau
Copy link
Collaborator

@apjanke

We talked about an OO approach for the repo at the last meeting and there was a "soft consensus" against it.

The main thing we would gain was encapsulation to hide the content of the layout structure from users. The cost of not using OO is that some users will go and "explore" that structure manually (see #63): we felt that this would be better resolved at the moment with some actual documentation on how to use the query function.

We also felt that that the switch to OO has another cost: that the contributor base in the Matlab / Octave world is not as familiar with OO as say people in the Python ecosystem. So it would put raise the bar of entry for contribution and / or put extra work on our side for on-boarding.

So I am going to close this... for now. As you never know what the future might bring.

@Remi-Gau Remi-Gau closed this Nov 13, 2020
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.

None yet

2 participants