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

Modifications to run on openmind, refactoring, and auto-formatting. #10

Closed
wants to merge 10 commits into from
Closed

Conversation

nwatters01
Copy link
Collaborator

Summary of changes:

  • This code now runs on openmind, the cluster where the data is stored. The data directories are different on openmind, so this required a few changes to paths.
  • A small change to the timescale transform (removing the bias), since I've modified the behavior in the dataset to eliminate the need for a bias.
  • Some renaming and refactoring, particularly in the main script.
  • Some auto-formatting.
  • Associated modifications to the README.

@CodyCBakerPhD
Copy link
Member

Something I just noticed @nwatters01

The new separation of the interfaces into the timeseries_interfaces.py file, but behavior_interfaces.py is still around, resulting in duplication of the interfaces (and some slight differences with the new ones?) making it confusing which one is supposed to be used

Did you mean to delete the original behavior_interfaces.py in this refactor?

reward_line = TimeSeries(
name='reward_line',
data=H5DataIO(self._reward_line, compression='gzip'),
unit='reward line open',
Copy link
Member

Choose a reason for hiding this comment

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

The unit field is generally something expected to be in SI units (no check yet on the Inspector due to difficulty agreeing on an exact substandard of text implementation of complex unit combinations NeurodataWithoutBorders/nwbinspector#208)

What does this new data stream look like? I don't recall seeing it in the initial round of example data

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a binary array. 1 means reward line is open, 0 means reward line is closed. You're right it was not in the initial example data, but I think for open-source data it's important to include exactly when the reward line was open and closed (sometimes monkey was given reward for reasons other than it's response, e.g. for fixating), hence why I added this.

What do you suggest we do with this data stream?

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks for explaining. That sounds like some great data to include!

I'd strongly recommend using a LabeledEvents structure from ndx-events instead of a generic TimeSeries for this type of data

Let me know if any questions about how to adapt to that alternate structure

@CodyCBakerPhD
Copy link
Member

@nwatters01 The summary of changes seemed to miss an important aspect: you also added two new data interfaces for data streams we haven't seen before

So I'm giving that side of this PR the usual more rigorous look over for best practices, etc.

BTW how do you feel about isort as a pre-commit?

@nwatters01
Copy link
Collaborator Author

nwatters01 commented Dec 18, 2023

@CodyCBakerPhD Thank you for the review! I really appreciate all comments and suggestions --- since this serves as a template conversion library for the lab, I absolutely want it to adhere to all of your best practices!

Yes I meant to remove behavior_interface.py. Just did that and am updating the PR.

And yes I added the reward and audio data streams. I've never used isort before, but if you prefer using that, I'm happy to give it a try --- what does it offer?

@CodyCBakerPhD
Copy link
Member

And yes I added the reward and audio data streams. I've never used isort before, but if you prefer using that, I'm happy to give it a try --- what does it offer?

Orders global imports by (a) those belonging to Python standard, (b) external packages, (c) locally defined modules

We have always followed this convention ourselves ($\pm$ human error) albeit manually, isort just runs in the background with the other pre-commits such as black

@nwatters01
Copy link
Collaborator Author

Oh, that sounds good. I'll apply isort and update the PR.

@CodyCBakerPhD
Copy link
Member

@nwatters01 One small conflict here now - also I didn't realize the pre-commit bot wasn't even running yet on this repo so I enabled that

@nwatters01
Copy link
Collaborator Author

I ran into a github rabbit hole trying to merge this repo with my fork, so instead I made a branch "nwatters" to this repo with all of these changes.

I think at this point the only thing I need to do is change to LabeledEvents structure, then that branch will be ready for merging.

@nwatters01
Copy link
Collaborator Author

In light of the github troubles I'm having with my fork, can we delete this pull request and switch to this one?

#13

That has all the same changes, but is actually merge-able into the main repo.

@CodyCBakerPhD
Copy link
Member

replaced by #13

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.

3 participants