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

Refactor contact_vector to _probe_* properties #3629

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alejoe91
Copy link
Member

@alejoe91 alejoe91 commented Jan 20, 2025

Fixes #3628

Open for discussion!

@alejoe91 alejoe91 added the core Changes to core module label Jan 20, 2025
@alejoe91 alejoe91 requested a review from samuelgarcia January 20, 2025 09:12
@alejoe91
Copy link
Member Author

Ok this is actually not as easy as it should be. The problem is the conctact_vector: when dumping to json, the complex dtype is lost and this messes up reloading.

Since we discussed about it, I propose we make a change as follows (but we keep back-compatibility):

  • instead of adding the contact_vector property, we add individual properties as _probe_{contact_vector_field}
  • we add helper functions to the baserecordingsnippet to go from list of properties to complex array and back

@samuelgarcia @zm711 what do you think?

@alejoe91 alejoe91 changed the title Expose include_properties in dump functions and set it to True Refactor contact_vector to _probe_* properties Jan 20, 2025
@alejoe91 alejoe91 marked this pull request as draft January 20, 2025 14:56
@h-mayorquin
Copy link
Collaborator

instead of adding the contact_vector property, we add individual properties as probe{contact_vector_field}

+1 on this.

I feel that contact_vector is not as clear as probe.

@alejoe91
Copy link
Member Author

This is something we should discuss at our next meeting :)

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

Successfully merging this pull request may close these issues.

Unexpected: AssertionError: 'inter_sample_shift' is not a property!
2 participants