Skip to content

Conversation

h-mayorquin
Copy link
Contributor

I worked on Blackrock today because of issue #1755 plus another user problem that we encountered on the past NWB workshop and there is an improvement I would like to make.

The current codebase uses generic variant names (variant_a, variant_b, variant_c) throughout the BlackrockRawIO implementation, making it difficult for developers to understand which Blackrock file specification version each function handles. When debugging or maintaining code, developers need to:

  1. Look up mapping dictionaries to understand what variant_a means
  2. Cross-reference specification versions with variant letters
  3. Remember arbitrary letter-to-version associations

I think is better to get rid of the arbitrary a, b and c and just include the specification directly on the name:

- variant_a → spec_v21 (handles specification 2.1)  
- variant_b → spec_v22_30 (handles specifications 2.2, 2.3, and 3.0)  
- variant_c → spec_v30_ptp (handles specification 3.0 with PTP timestamps)  
- Special cases: spec_v21_22 and spec_v23 for NEV data functions with specific version ranges  

Debugging then becomes easier because the function name directly shows the specification context with the intent clear from the function signature alone.

All of these are private functions so no problem with internal name changing (god bless private functions)

What do you think?

@h-mayorquin h-mayorquin self-assigned this Sep 5, 2025
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

@zm711 zm711 added this to the 0.14.3 milestone Sep 8, 2025
@zm711
Copy link
Contributor

zm711 commented Sep 8, 2025

You good with me merging this? Or any other tweaks you want to make?

@h-mayorquin h-mayorquin merged commit 3b592a8 into NeuralEnsemble:master Sep 8, 2025
3 checks passed
@h-mayorquin h-mayorquin deleted the fix_black_rock_1 branch September 8, 2025 15:56
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.

2 participants