Open bahanonu opened 3 years ago
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.
I like namespaces as much as the next Python programmer, but this seems like a lot.
@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.
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).
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:
misc.getMatnwbDir
/ matnwb.misc.getMatnwbDir
will still work as intendedsprintf
or other cases where only partial package names of a function or class is coded explicitly.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
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.nwbtest.m
needs to search from the matnwb.tests
package.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!
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.
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
Oh wow, I would not have expected that. Excellent job.
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?
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
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);
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.
@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.
@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)
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.
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:
src
in python) in one place, to make it easier to create matlab toolbox files (.mltbx
) for releases.tools
folder to have a clear separation of concerns
Is there any plan to move the current
matnwb
packages into sub-packages of amatnwb
package for a cleaner namespace?For example, instead of calling
types.core.OpticalChannel
users could callmatnwb.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 hastypes= '';
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.