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

matnwb package #262

Open
bahanonu opened this issue Feb 3, 2021 · 17 comments
Open

matnwb package #262

bahanonu opened this issue Feb 3, 2021 · 17 comments
Assignees
Labels
category: enhancement improvements of code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users status: todo something needs to be done topic: matnwb-api related to improving the matnwb api

Comments

@bahanonu
Copy link
Contributor

bahanonu commented Feb 3, 2021

Is there any plan to move the current matnwb packages into sub-packages of a matnwb package for a cleaner namespace?

For example, instead of calling types.core.OpticalChannel users could call matnwb.types.core.OpticalChannel and this would be true for all functions in the current top-level packages.

This would also reduce potential conflicts given the names of the current top-level packages—whereas currently, matnwb may have to be added or remove from the path each time a NWB read/write operation is performed to avoid this (e.g. if a parent function has types= ''; or similar command, issues could arise).

This would not be a minor change (since other packages and code depend on the current setup), but may be worth considering.

@lawrence-mbf lawrence-mbf self-assigned this Feb 3, 2021
@lawrence-mbf
Copy link
Collaborator

No but your timing is pristine as I can say that the alternative table constructor in native MATLAB does break for precisely this namespace reason. Evidently, the script used for the internal tabular class uses a variable called types which interferes with regular MatNWB use, especially when allocating space using ObjectViews.

You are correct that this will be quite a bit of effort as not only does the code regularly reference other functions within itself, the generated class locations will all need to be changed, and there will definitely be breakage in the utility scripts and other third-party applications that utilize the generated classes and custom types.

@bendichter
Copy link
Contributor

I like namespaces as much as the next Python programmer, but this seems like a lot.

@bahanonu
Copy link
Contributor Author

bahanonu commented Feb 4, 2021

@ln-vidrio
Thanks for the heads up about table, that's a good example case.

It is possible depending on the timescale of implementation, users could be notified of the change (as MathWorks currently does) and that new functions would be put under the matnwb package (with several ways that would allow a gradual transition of existing functions to a matnwb package).

For existing users, if a matnwb package is made, many would likely just need to start functions/scripts with import matnwb.* if they want to use the current naming conventions.

Else can just rely on adding/removing matnwb from path as needed to avoid these issues.

@ehennestad
Copy link
Collaborator

ehennestad commented Feb 1, 2024

I support this!

FYI:

Matlab provides an option to define an alias file, which supports backwards compatibility of names.

The matlab.alias.AliasFileManager class provides an API for creating and maintaining alias definitions. Aliases are not part of class definitions. Instead, alias definition files are stored in resources folders that are located in the same folder as the latest class file.

This would ensure that utility functions and third party applications would continue to work as is (for a transition period, lets say).

@ehennestad
Copy link
Collaborator

ehennestad commented Feb 1, 2024

I gave this an attempt here: https://github.com/ehennestad/matnwb

All the packages are moved into a main matnwb package, and all the package prefixes I could find have been renamed accordingly.

I ran the generateCore again, and it generates types with correct new names.

A few known issues that need further testing/investigating:

  • I did not investigate fully whether functions depending on the misc.getMatnwbDir / matnwb.misc.getMatnwbDir will still work as intended
  • I have not run any tests
  • I don't know if I have found all cases where a classname is built dynamically by joining package prefixes using e.g sprintf or other cases where only partial package names of a function or class is coded explicitly.
  • Documentation is not updated

Note: I also found a few references to classes that appears to have moved from types.core to types.hdmf_common. See here: d11751b and here: 04fbecf

Final note:
The alias.json will only work with classes but I assume most external software will work with the types classes

@lawrence-mbf
Copy link
Collaborator

I did not investigate fully whether functions depending on the misc.getMatnwbDir / matnwb.misc.getMatnwbDir will still work as intended

You would need to add another fileparts(matnwbDir) after line 9 but that should resolve it for all cases.

I have not run any tests

  • tests.system.PyNWBIOTest line 39 should add +matnwb to the top level search.
  • The nwbtest.m needs to search from the matnwb.tests package.
  • ignoreFolders and ignorePaths need to prepend matnwb to their paths as well. I think everything else will end up being automated.

I don't know if I have found all cases where a classname is built dynamically by joining package prefixes using e.g sprintf or other cases where only partial package names of a function or class is coded explicitly.

This is only really done in file generation so if that is successful it might be fine though that will require running tests.

Looks awesome!

@ehennestad
Copy link
Collaborator

Thanks! I think I got everything sorted out for running the tests.

Very many of the tests fail with this error:
Values for property 'file_create_date' are not equal
The value appears to be exactly one month wrong. I did add breakpoints in the NWB file when the date is created and then it appears correct, so this is something I don't understand.

Another issue I have is that the python tests don't succeed I assume because I don't have pynwb installed.

@ehennestad
Copy link
Collaborator

I tracked down the date issue to this function io.timestamp2datetime

This code snippet illustrates the behavior:

DatetimeCorrect = datetime(0, 0, 0, 0, 0, 0, 0);
DatetimeIncorrect = datetime(0, 0, 0, 0, 0, 0, 0);

ymdStamp = '2024-02-02';

YmdToken = struct(...
    'Year', ymdStamp(1:4),  ...
    'Month', ymdStamp(6:7), ...
    'Day', ymdStamp(9:10) );

DatetimeCorrect.Day = str2double(YmdToken.Day);
DatetimeCorrect.Month = str2double(YmdToken.Month);
DatetimeCorrect.Year = str2double(YmdToken.Year);

DatetimeIncorrect.Year = str2double(YmdToken.Year);
DatetimeIncorrect.Month = str2double(YmdToken.Month);
DatetimeIncorrect.Day = str2double(YmdToken.Day);

Turns out setting the month before the day can flip the month value one step ahead. The default value for datetime(0, 0, 0, 0, 0, 0, 0) is 30-nov, and when setting the month to February, since February does not have 30 days the month and day is adjusted automatically to 01-Mar. So running tests in February is not a good idea :)

Setting the day before the month solves it

@lawrence-mbf
Copy link
Collaborator

Oh wow, I would not have expected that. Excellent job.

@ehennestad
Copy link
Collaborator

Yeh, thats one of the weirdest bugs ive encountered. Could you give me a brief explanation what is needed to run python tests? Or point me to documentation if that exists?

@lawrence-mbf
Copy link
Collaborator

lawrence-mbf commented Feb 2, 2024

If your python env is on your path, I think the tests should automatically use it.

If not, you can save an env.mat file with a pythonDir name in it that contains a full path to the directory containing your python binary. Running tests using nwbtest should then run the python compatibility tests.

For more information you can take a look at +matnwb/+tests/+system/PyNWBIOTest.m and its python test script equivalent +matnwb/+tests/+system/PyNWBIOTest.py

@vijayiyer05
Copy link

vijayiyer05 commented Feb 5, 2024

Turns out setting the month before the day can flip the month value one step ahead. The default value for datetime(0, 0, 0, 0, 0, 0, 0) is 30-nov, and when setting the month to February, since February does not have 30 days the month and day is adjusted automatically to 01-Mar. So running tests in February is not a good idea :)

Thanks for elucidating. I'm open to submitting this internally, but I think folks would stumble on why it's mixing & matching datetime w/ other types.

At a glance, I think the desired correct behavior might be achieved compactly in this way:

ymd = datetime(2024,2,2);
correctYear = year(ymd);
correctMonth = month(ymd);
correctDay = day(ymd);

@ehennestad
Copy link
Collaborator

Hi Vijay,
I don't consider this as a bug in MATLAB! The issue was with an internal function in matnwb (io.timestamp2datetime) where a string is converted to a datetime value.

@bahanonu
Copy link
Contributor Author

@rly @bendichter Following up from the NWB Workshop about converting matnwb so all functions are inside a package (similar to what @ehennestad has done at https://github.com/ehennestad/matnwb). Happy to help test.

@vijayiyer05
Copy link

@rly @bendichter Following up from the NWB Workshop about converting matnwb so all functions are inside a package (similar to what @ehennestad has done at https://github.com/ehennestad/matnwb). Happy to help test.

+1 on this point/issue. One recent/emergent advantage of root namespaces: they enable a toolbox's live script example(s) to be run straight from an Open in MATLAB Online link, as alluded at the end of this blog post. You can this workflow in action at the Brain Observatory Toolbox repository.

Additionally, it makes sense to me for NWB to lead way wrt use of root namespaces (such as matnwb or even, dare I say, nwb*). Because it would be excellent, for instance, for various algorithmic packages to have standardly (redundantly) named functions like 'toNWB' & 'fromNWB`.

(*) Personally I'm open to the possibility that three-letter acronym (TLA) namespaces can work in a future where compute environments are often configured to their specific domains (with containers or otherwise)

@ehennestad
Copy link
Collaborator

ehennestad commented Jun 20, 2024

Following up from the NWB Workshop about converting matnwb so all functions are inside a package (similar to what @ehennestad has done at https://github.com/ehennestad/matnwb). Happy to help test.

I think what I have done so far was complete at the time of my latest commit, but I did not have the time to get into thorough testing on real-world nwb files.

I am also open to considering another namespace like nwb, although updating the tutorials was quite laborious, as there are a lot of links in there that needs to be updated manually and individually.

@ehennestad ehennestad added category: enhancement improvements of code or code behavior status: todo something needs to be done topic: matnwb-api related to improving the matnwb api priority: medium non-critical problem and/or affecting only a small set of NWB users labels Oct 31, 2024
@ehennestad
Copy link
Collaborator

Suggestion to also reorganise the folder structure of the root directory along the following lines:

/
├── .github
├── code
│   ├── +matnwb
│   ├── external_packages
│   ├── jar
│   ├── nwb-schema
│   ├── tutorials
│   ├── NwbFile.m
│   ├── functionSignatures.json
│   ├── generateCore.m
│   ├── generateExtension.m
│   ├── nwbClearGenerated.m
│   ├── nwbExport.m
│   ├── nwbRead.m
├── docs
│   └── documentation
├── resources
└── tools
│   ├── tests
│   └── nwbtest.m
├── .gitignore
├── LICENSE
├── README.md

Main goals:

  • Place all code (equivalent of src in python) in one place, to make it easier to create matlab toolbox files (.mltbx) for releases.
  • Place all tests and other developer tools in a separate tools folder to have a clear separation of concerns

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 priority: medium non-critical problem and/or affecting only a small set of NWB users status: todo something needs to be done topic: matnwb-api related to improving the matnwb api
Projects
None yet
Development

No branches or pull requests

5 participants