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]: do we still need to require user to generateCore();? #555

Closed
2 tasks done
bendichter opened this issue Dec 20, 2023 · 6 comments · Fixed by #556
Closed
2 tasks done

[Feature]: do we still need to require user to generateCore();? #555

bendichter opened this issue Dec 20, 2023 · 6 comments · Fixed by #556
Assignees

Comments

@bendichter
Copy link
Contributor

What would you like to see added to MatNWB?

@lawrence-mbf, this is a question that @rly brought up in our last meeting and I am also curious.

Do we still need to require users to call generateCore() to install MatNWB? What if we pre-generated the classes for the latest schema? This would make the installation process simpler.

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 lawrence-mbf self-assigned this Dec 20, 2023
@lawrence-mbf
Copy link
Collaborator

We should probably adjust that requirement. You only need to call generateCore() if you are writing NWB files from scratch.

If you are modifying or otherwise just reading files, you only need nwbRead since that already calls generateCore in the background.

@lawrence-mbf
Copy link
Collaborator

Actually, thinking about it, there are two minor caveats to including pregenerated classes in the repo:

  1. If the user is going to only use the repo to read files, it's a (admittedly minor) waste of bandwidth and overwrite time.
  2. More importantly, if the user has installed the repository in a place that they cannot write to or if the user is trying several different environments at once, then keeping pregenerated files in the repo may cause issues as MATLAB names on the PATH are higher priority than whatever is in the cwd. You will end up with strange class errors because MATLAB is preferring, say, a TimeSeries object from our pregenerated cache over whatever the user has set in cwd or even a different path. I do not know how many users actually manage their environments in this way but I seem to recall running into issues like this for certain tools that use MatNWB like Brainstorm.

@bendichter
Copy link
Contributor Author

  1. I don't think this is a problem. It's just a few text files, and the size is completely reasonable for a library installation. If we really want to optimize repo size, we should focus on the html files, which are much bigger and having them local is providing no benefit to normal users.

  2. How could the user install this repo to a location where they don't have write permissions? If that is the case they won't be able to execute the git clone command?

I think given our use of namespaces, it is unlikely that a user would want to use another type called e.g. types.core.TimeSeries, unless they are trying to use different versions of MatNWB, in which case they can do so by manipulating their path. Is that the case? Am I missing a critical use-case here?

@lawrence-mbf
Copy link
Collaborator

I would advise not manipulating the MATLAB path for this reason because of the unintuitive nature of path resolution (cwd not overriding things on PATH). Rather, the user should probably be using generateCore('schema version') instead and switching as necessary. Other than that this sounds fine. We can include version 2.6.0 into the default namespace.

@bendichter
Copy link
Contributor Author

I'm not really understanding the use case you have in mind where this would pose a problem

@lawrence-mbf
Copy link
Collaborator

I mean, I would trust your judgement over this being a non-issue over mine since you probably use this tool more than I do.

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

Successfully merging a pull request may close this issue.

2 participants