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

Toolbox on the narrowest grounds possible #11

Open
gkaguirre opened this issue Oct 31, 2017 · 6 comments
Open

Toolbox on the narrowest grounds possible #11

gkaguirre opened this issue Oct 31, 2017 · 6 comments

Comments

@gkaguirre
Copy link
Contributor

@DavidBrainard knows that this is a bugaboo of mine.

As we develop the OLApproach repos, I would like us to examine the JSON files and modify the toolbox includes to have them include ONLY the code that is being used. As a general principle, a directory of code should not be added to the path if the repo doesn't actually use anything in the that directory.

So, the OLApproach routines currently add the BrainardLabToolbox and the Psychtoolbox-3 to the path. Can we prune these down to just the sub-directories that contain routines that are actually being used?

@JorisVincent
Copy link
Contributor

JorisVincent commented Feb 15, 2018

I had a quick look at this, but it requires rewriting some things in ToolboxToolbox.

Firstly, there's a bug in how it reads in the subfolders field in the records: in the JSONs they're stored as string arrays - which are not cell-arrays of strings - which makes tbDeployToolboxes think that no subfolders are specified. Thank you Matlab for having 4 different ways and 3 basic types for storing strings. This problem can be see in the OneLightToolbox.JSON, which quite clearly does not want to include any of the x-prefaced subdirectories (e.g., xOld); however, these are still included. This is quite easily fixed.

More problematic is that a toolbox often defines in its own record, which subdirectories to add. In the case of Psychtoolbox-3, it specifies that the entire Psychtoolbox-3/Psychtoolbox subdirectory must be added to path. Thus, even if, e.g., the OneLightToolbox requests only to add Psychtoolbox-3/Psychtoolbox/PsychCal, the psychtoolbox itself request that the entire toolbox gets added.
This behavior makes some sense: presumably the psychtoolbox "knows better" which subdirectories are necessary. On the other hand, adding the entire psychtoolbox is clearly not necessary. What it really comes down to, is that many of our toolboxes are, in essence, a collection of loosely dependent functions / sub-toolboxes...

@gkaguirre
Copy link
Contributor Author

Thanks for looking at this Joris. It might be tough to prune the Psychtoolbox install. How about the BrainardLabToolbox? Do we know what elements / subdirectories are actually used?

@JorisVincent
Copy link
Contributor

JorisVincent commented Feb 16, 2018

Matlab's builtin matlab.codetools.requiredFilesAndProducts(filename) function will list all the depedencies for a given function. Since these are plethora (257 for the PurkinjeTreeDemo), I've written a function that parses this list.

The function below finds toolboxes that TbTb knows about, and for each of those the exact subdirectories (i.e., down to the sub-sub-subdirectory that a function is in) that are required. It can also spit out the names of the required m-files. Since it could be that not all dependencies are managed by TbTb (or they are in projects, which are a hassle to check...), it can also spit out unresolved dependencies. The one thing it currently does not do, is trace where/how it uses each function.

Maybe we want this in the TbTb (@DavidBrainard)? I could probably write some more code that batch-runs this on everything in a project/toolbox folder, collects the output, and writes a TbTb configuration....

tbDependencies.txt (download and rename to tbDependencies.m to run)

@JorisVincent
Copy link
Contributor

JorisVincent commented Feb 16, 2018

Short answer, of the BrainardLabToolbox the OLApproach_Psychophysics/RunDemo uses:

  • Classes/@GamePad
  • ExperimentSupport
  • OOCalibrationToolbox/@PR650dev
  • OOCalibrationToolbox/@PR670dev
  • OOCalibrationToolbox/@Radiometer
  • OneLiners
  • Overrides/PTB-3/PsychCal
  • Overrides/PTB-3/PsychCal/@CalStruct
  • PathUtilities

(ran [toolboxes, subdirs] = tbDependencies('RunDemo'), then subdirs('BrainardLabToolbox'))

@JorisVincent
Copy link
Contributor

I've pushed an updated config .json to our fork of the ToolboxRegistry (https://github.com/BrainardLab/ToolboxRegistry/commit/a73555d13b004b7c335a351f637fb09d46b6738c). This new config defines at least the OneLightDriver, BrainardLabToolbox, SST and PTB as dependencies of the OneLightToolbox. We definitely need all of those (though not necessarily all code in each dependency).

I tried to use the subfolders argument for the BrainardLabToolbox-dependency, but it looks like that doesn't work (all the subfolders of BrainardLabToolbox get added).

@DavidBrainard
Copy link
Collaborator

Issue a PR and we can get it into the "official" copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants