-
Notifications
You must be signed in to change notification settings - Fork 3
Open Questions
-
There is a lot of commonality between the
OphysManifestclass and theEphysManifestclass. It would make sense to gather this in a superclass, and theManifestclass is an obvious candidate. Would that be ok to do, or would that violate some principles of design?- VI: Much of OOP design pattern lore guides to "favor association over inheritance", which is what's done currently. However I believe this lore largely stems from Java & strongly reflects its lack of multiple inheritance. I agree with your instinct to favor inheritance where rational such as here. Seems a worthwhile refactor to consider. And regardless of association or inheritance relationship, refactoring to centralize common functionality (e.g.
updateManifestas noted below) would be worthwhile. As your next question gets at, this may involve detective work to determine what is or could/should be common.
- VI: Much of OOP design pattern lore guides to "favor association over inheritance", which is what's done currently. However I believe this lore largely stems from Java & strongly reflects its lack of multiple inheritance. I agree with your instinct to favor inheritance where rational such as here. Seems a worthwhile refactor to consider. And regardless of association or inheritance relationship, refactoring to centralize common functionality (e.g.
-
The
Ephys- andOphysManifestclasses do not have the same internal logic for retrieving and caching manifests. In theOphysManifest, all the different tables are fetched and put into a struct and immediately added to the disk cache, whereas in theEphysManifestthey are handled more individually and the final processed tables are initially cached in memory, and only added to the disk cache when the property get method for particular item types are invoked. Is there a particular reason for this (i.e performance considerations during retrieval)?- VI: Great that you're analyzing this. Perhaps DM has insights.
-
Would it be cleaner to use the
OnDemandPropertiesmixin for memory caching of tables in theManifestclasses?- VI: So far OnDemandProperties has only been used for user-facing properties of item object properties. I guess adapting to private properties would be fairly trivial, but would have to analyze wrt to how intertwined/specialized it is for item objects today. Conceptually seems sound, but would tend to prioritize lower if it doesn't prove highly straightforward.
-
What is the purpose of the
UpdateManifestsmethod - Why is it called update? It seems more like a clear/reset method...- VI: I believe it was intended to allow a fresh check of the data source (whether API or now possibly S3), which is always subject to change (which has occurred from time to time during the project, affecting demos etc). Maybe "Refresh" would have been a better word than "Update", but the implementation is a clear/reset type operation as you say.
- In the
bot.internal.cache/CachedApiCallmethod, there are a lot of optional arguments, like:nPageSize,strFormat,strRMAPrefix,strHost,strScheme. However, none of the calls to this method makes use of them, and for some, likestrScheme,strHostandstrRMAPrefix, it seems like they are not really variable in the context of the Allen Brain Observatory Api. Was there any past scenario where these were useful, or is there any future scenario where they might be put to use? - Do you know if there exists documentation for the RMA API that is specific for the brain observatory? Or is the current API calls in matlab derived from the python toolbox?
- Is there a reason why behavioral data and pupil data are not added as linked files? Are they not available as
well_known_filesfrom the API?
- Are there H5 files in the Visual Coding 2P dataset at this point? The "SessH5" linked file appears defunct in various ophys sessions I've retrieved lately.