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

Subset dev #14

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Subset dev #14

wants to merge 14 commits into from

Conversation

aplested
Copy link
Contributor

allow selection of a subset of traces for idealisation

@aplested aplested requested a review from eckuru March 21, 2022 08:49
## self.idealize_series()
event_array = np.zeros((0, 5)).astype(object)
for episode in self.data.series:
if self.idealization(episode.n_episode) is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the same as if self.idealization(episode.n_episode): so you don't have to use is not None at the end. Python treats None as well as empty list ([]) and empty string ('') as False in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Maybe I put an explanatory comment.

Comment on lines 142 to 143
if datakey is None:
datakey = self.current_datakey
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another unimportant tip/comment: you can do this shorter like datakey = datakey or self.current_datakey.

src/core/recording.py Outdated Show resolved Hide resolved
=======
elif filetype == "pkl":
return_status = save_pickle(data=data, filepath=filepath)
>>>>>>> subsets
else:
print('Can only save as ".mat"!')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or as ".pkl" now :).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait, actually no. The save_pickle function this is referring to is still commented out. Nevermind!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it again

selected_subsets,_ = self.main.ep_frame.subset_frame.subsets_check()
episodes = self.main.data.episodes_in_subsets(selected_subsets)
for ep in episodes:
print (f"Idealizing episode {ep.n_episode}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't know this also worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this is something should react to or not!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a catch now for the case that the list returned is empty because nothing was checked, with an error message to the terminal.

)
else:
self.parent.main.data.subsets[name][0].remove(index)
n = self.parent.ep_list.item(index).text()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You overwrite this right away... I'm generally a bit confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true, very messy isn't it. I commented it out.

@eckuru
Copy link
Collaborator

eckuru commented Mar 31, 2022

I now looked at all the files, haven't yet run it myself. There are some minor changes to clean up that I had merged before, and merging this branch will reintroduce those, so it would be good to remove them. I think it should be possible to merge master locally and fix those and push, instead of doing manually. I usually can't guess right if git will think something is a conflict though.

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