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

rmv unnecessary file prefix with conversion name #7

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bendichter
Copy link

fix #5

@CodyCBakerPhD
Copy link
Member

I think we had it this way for the same reason DANDI attaches sub-<subject_id> and ses-<session_id> to the names of files in a dandiset even when they're already nested in folders with that naming pattern; if those files ever get accidentally moved or copied elsewhere, trying to figure out where they belong can be a real pain

Also specific to the nwbconverter.py and behaviorinterface.py, the name of those files is intended to follow the convention where the filename matches the class defined inside, and those classes really ought to be named specific to their experiment type unless they're super general to the lab (in which case they should still be named in a way that makes that clear)

@bendichter
Copy link
Author

I think we had it this way for the same reason DANDI attaches sub-<subject_id> and ses-<session_id> to the names of files in a dandiset even when they're already nested in folders with that naming pattern; if those files ever get accidentally moved or copied elsewhere, trying to figure out where they belong can be a real pain

That makes some sense for data, where you might imagine the user moving files around, but not really for code. I am not aware of any software libraries that follow this kind of naming convention. Are you?

@CodyCBakerPhD
Copy link
Member

It's analogous to how DataJoint appends modality identifiers to shared node names across their pipelines: https://github.com/datajoint/element-array-ephys/tree/main/element_array_ephys vs. https://github.com/datajoint/element-calcium-imaging/tree/main/element_calcium_imaging for no_curation, pre_process, and report.

Even though they have the code for each pipeline in a different repo it would be possible for a user/lab to have multiple pipelines nested relatively close together which would produce a structure similar to what we see here

@bendichter
Copy link
Author

I don't think this is that close. In our case, you have prepended each of the files in a directory with the name of the folder they are contained within. In their case, they have a naming convention for modules that is {modality}_{type}, so there is some commonality among files that have the same modality.

@h-mayorquin
Copy link
Collaborator

I did not see this before. While doing the current conversion I realized that the past structure I was using is too verbose. I also prefer the more concise standard that Ben is proposing here. The folder structure and class names are enough for disambiguation I feel.

I hear what @CodyCBakerPhD says about having identical files across the repo being more prone to confusion but I also feel that as long as the classes inside are prepend with the name of the conversion like:

{{cookiecutter.conversion_name_camel_case}}BehaviorInterface

It should be all right as no file in the wrong place will run (that said, this structure is still annoying for me in my IDE as makes navigation a bit more difficult but that's probably a problem with my IDE that should have no bearing on the organization of a file system).

Another point that @bendichter has not brought up is that this makes documenting package usage a bit easier when everything inside of each (simple) conversion looks more or less the same and is only the directory / folder that varies for conversion and project.

@@ -47,7 +58,8 @@ Note:
both of the methods above install the repository in [editable mode](https://pip.pypa.io/en/stable/cli/pip_install/#editable-installs).

### Running a specific conversion
To run a specific conversion, you might need to install first some conversion specific dependencies that are located in each conversion directory:
To run a specific conversion, you might need to install first some conversion specific dependencies that are located
in each conversion directory:
```
pip install -r src/{{cookiecutter.repo_name_slug}}/{{cookiecutter.conversion_name}}/{{cookiecutter.conversion_name}}_requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pip install -r src/{{cookiecutter.repo_name_slug}}/{{cookiecutter.conversion_name}}/{{cookiecutter.conversion_name}}_requirements.txt
pip install -r src/{{cookiecutter.repo_name_slug}}/{{cookiecutter.conversion_name}}/requirements.txt

Right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following line should be changed in this file as well:
editable_metadata_path = Path(file).parent / "{{cookiecutter.conversion_name}}_metadata.yaml"

@@ -1,3 +0,0 @@
nwb-conversion-tools==0.11.1 # Example of specific pinned dependecy
some-extra-package==1.11.3 # Example of another extra package that's necessary for the current conversion
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you feel that the other examples were too much detail?

some-extra-package==1.11.3  # Example of another extra package that's necessary for the current conversion
roiextractors @ git+https://github.com/catalystneuro/roiextractors.git@8db5f9cb3a7ee5efee49b7fd0b694c7a8105519a # Github pinned dependenc

I like the last one as a reminder to pin to specific commit of github for roiextractor or spikeinterface. When doing past projects they were changing quickly but maybe this is not a good practice anymore.

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 this pull request may close these issues.

rmv redundant prefix
3 participants