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 redundant prefix #5

Open
bendichter opened this issue Sep 14, 2022 · 6 comments · May be fixed by #7
Open

rmv redundant prefix #5

bendichter opened this issue Sep 14, 2022 · 6 comments · May be fixed by #7

Comments

@bendichter
Copy link

bendichter commented Sep 14, 2022

eric-kandel-lab-to-nwb/
├── LICENSE
├── make_env.yml
├── pyproject.toml
├── README.md
├── requirements.txt
├── setup.py
└── src
    ├── eric_kandel_lab_to_nwb
    │   └── brenda_milner_2022
    │       ├── brenda_milner_2022behaviorinterface.py
    │       ├── brenda_milner_2022_convert_script.py
    │       ├── brenda_milner_2022_metadata.yml
    │       ├── brenda_milner_2022nwbconverter.py
    │       ├── brenda_milner_2022_requirements.txt
    │       └── __init__.py
    └── __init__.py

Can we remove the "brenda_milner_2022" prefix from brenda_milner_2022behaviorinterface.py etc.? It's redundant if these files are already in the brenda_milner_2022 directory

@bendichter
Copy link
Author

Can you also not use real neuroscientist's names? It might imply an endorsement that we don't really have. Could you use fictional or historical characters? Like Plato or Bruce Banner

@CodyCBakerPhD
Copy link
Member

Can you also not use real neuroscientist's names? It might imply an endorsement that we don't really have. Could you use fictional or historical characters? Like Plato or Bruce Banner

Yeah that's a good point, we should not be doing that. I always endorse the classic anonymous naming scheme for this kind of thing: John & Jane Doe. Not as fun as Marvel characters, I know, but it's the most professional/legal IMO.

@bendichter
Copy link
Author

yeah, we could probably get away with it but no need to cause any potential legal issues with Disney. Maybe stick to public domain, historical, or stock names like Jane Doe

@CodyCBakerPhD
Copy link
Member

As for the heart of the question on this issue, I believe the justification is two-fold:

(a) The name of the Python file mirrors the name of the single class definition that it contains, just like any of our interfaces in the core NeuroConv. That is, the file /neuroconv/datainterfaces/ecephys/spikeglx/spikeglxdatainterface contains the SpikeGLXDataInterface (OK, technically the actual name of the class is SpikeGLXRecordingInterface, but for custom interfaces in a conversion project they are usually just a DataInterface or a <DataType>Interface like above).

This class should always have a prefix that indicates it's specific use to the very specific experiment setup, and will typically mirror the prefix of the corresponding custom NWBConverter class. Otherwise, if you just named such things BehaviorInterface and relied on submodule locale to determine the specify, I could foresee all kinds of confusing mistakes resulting from such ambiguity that would take more time than necessary to debug. Better to just include a bit more extra information ahead of time and avoid the situation IMO

(b) Given (a), if the file containing this class became misplaced accidentally (even within the same project) it is easier to overlook such a misplacement when the file stem itself has something that indicates this specificity rather than having to introspect the file contents to guess. This philosophy is similar to the DANDI organization, where the name of each NWB file contains the session and subject IDs for that file in addition to the file being nested within folders whose names also contain that information

@CodyCBakerPhD
Copy link
Member

Also worth mentioning, with regards to (a) in the past I've always tried to use the smallest prefix possible that keeps things in the repo unique, and can always be changed on the fly (keep is simple on first cookiecut, then extend over time on the repo).

This came up a lot in the Buzsaki repo where most experimenters only had one big paper that the data corresponded to, so I'd usually just set the prefix to be the last name of that experimenter.

Additional info would like year of the paper publication or differentiating subject matter of the paper would only be added to the last name if the author had or in this case was expecting to have more publications. So milner2022behaviorinterface is a bit less typing to that extent.

@bendichter
Copy link
Author

can we change this to:

doe-lab-to-nwb/
├── LICENSE
├── make_env.yml
├── pyproject.toml
├── README.md
├── requirements.txt
├── setup.py
└── src
    ├── doe_to_nwb
    │   └── milner2022
    │       ├── behaviorinterface.py
    │       ├── convert_script.py
    │       ├── metadata.yml
    │       ├── nwbconverter.py
    │       ├── requirements.txt
    │       └── __init__.py
    └── __init__.py

@bendichter bendichter linked a pull request Feb 28, 2023 that will close this issue
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