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

[Feature]: Use new name=val pattern #570

Open
2 tasks done
bendichter opened this issue Jun 13, 2024 · 6 comments
Open
2 tasks done

[Feature]: Use new name=val pattern #570

bendichter opened this issue Jun 13, 2024 · 6 comments
Labels
category: enhancement improvements of code or code behavior status: need verification potentially solved, but needs verification topic: docs related to documentation

Comments

@bendichter
Copy link
Contributor

What would you like to see added to MatNWB?

MATLAB now allows you to have the syntax func(a=1, b=2), which is nicer and would be more similar to PyNWB. It would be nice to look at converting the library to use this syntax.

Is your feature request related to a problem?

No response

What solution would you like?

see above

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

@lawrence-mbf
Copy link
Collaborator

Note that this syntax is only available in MATLAB R2021a which may impact users stuck or unwilling to upgrade to newer versions.

@bendichter
Copy link
Contributor Author

Yes, I have seen some applications where you can keep the old format as well, e.g. with plotting: https://www.mathworks.com/help/matlab/matlab_prog/namevalue-in-function-calls.html I'm not sure how we would implement this ourselves

@ehennestad
Copy link
Collaborator

It is already possible to use this syntax without changing anything in the library.

The question is, should tutorials be updated to use this newer syntax? And then the problem is, as @lawrence-mbf notes, that people using older versions would not be able to run the tutorials

@rly
Copy link
Contributor

rly commented Jul 13, 2024

PyNWB supports all currently alive/active versions of Python. Each version lasts five years. The documentation does not use features specific to only a subset of those versions.

I am in favor of updating the matnwb tutorials to use recently released MATLAB features that improve quality of life for the vast majority of users. Users running older versions can still use matnwb but would need to adjust the tutorial code. We could add a note about that in each tutorial, but this is pretty unfriendly to novices. I hope that this would impact relatively few users.

What should the time (or version) cutoff be? In my limited experience, when scientists start using a particular version of MATLAB for their data, they will not upgrade until the analysis is done, just in case the behavior of their code changes in later versions. Some people use older versions because their lab code doesn't run on newer versions. But they could also install a recent version of MATLAB to use matnwb without having to modify the tutorials. Three years sounds reasonable to me (which means I think we should update the tutorials to use the name=val pattern introduced in 2021a). Four or five years would be extra safe.

@rly
Copy link
Contributor

rly commented Jul 13, 2024

We could also just try it and roll it back if there are significant complaints.

@ehennestad ehennestad added category: enhancement improvements of code or code behavior topic: docs related to documentation status: todo something needs to be done labels Oct 31, 2024
@ehennestad
Copy link
Collaborator

ehennestad commented Nov 21, 2024

There is a special constructor mode for NWB types where values can be added to a set contained in the property of an object using name-value syntax in the class constructor.
For example:
position = types.core.Position('SpatialSeries', spatial_series_ts);

In this case, 'SpatialSeries' is not a property of the types.core.Position class, instead the spatial_series_ts will be added as a named value to the spatialseries property of types.core.Position, where spatialseries is a types.untyped.Set and 'SpatialSeries' is used as a name.

For this mode, the name=value syntax does not work. I therefore think it might be confusing to introduce the name=value syntax, as there will be cases where it will not work, and it is a quite technical explanation that I think users should not have to think about.

In my opinion, the best way forward would be to get rid of support for this special constructor mode.

A few reasons for this:

  • This mode prevents use of arguments blocks. arguments blocks unlocks autocompletion of name-value pairs and in general makes the code cleaner and more maintainable.
  • Passing name-value pairs to the constructor which is then added to a property with dictionary-like behavior behind the scenes is opaque and can easily cause confusion.
  • It is also not standard in MATLAB to pass name-value pairs to a class constructor where the name does not correspond to a property or options. When comparing the expression above with the class documentation, it is unclear to a user how an "arbitrary" name can be used as an input.
  • It deviates from how it is done in pynwb.

@ehennestad ehennestad added status: need verification potentially solved, but needs verification and removed status: todo something needs to be done labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior status: need verification potentially solved, but needs verification topic: docs related to documentation
Projects
None yet
Development

No branches or pull requests

4 participants