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

Make in-place selections the default for data access #175

Open
pantaray opened this issue Dec 16, 2021 · 8 comments
Open

Make in-place selections the default for data access #175

pantaray opened this issue Dec 16, 2021 · 8 comments
Assignees
Labels
Explore Examine novel functionality/proposed changes etc. Does not necessarily involve coding things. More info needed More information is needed from the person who reported this.
Milestone

Comments

@pantaray
Copy link
Member

pantaray commented Dec 16, 2021

Explore UI/UX options to have in-place selections be present "inherently" by injecting the selection in all relevant property getters. For instance, if data contains 128 channels, 92 of which are selected (in-place), then data.channel.size == 92. It might be a good idea to issue a SPYInfo message every time a property is accessed with an active in-place selection attached.

@pantaray pantaray added the Explore Examine novel functionality/proposed changes etc. Does not necessarily involve coding things. label Dec 16, 2021
@pantaray pantaray added this to the datatype milestone Dec 16, 2021
@pantaray pantaray self-assigned this Dec 16, 2021
@tensionhead
Copy link
Contributor

Is this still sth we want to achieve, so shall we move this to a dedicated project @pantaray ?

@pantaray
Copy link
Member Author

I'm not sure, to be honest. That's probably something we should play around with a little to decide if it even makes sense. So I don't see any current project this really fits in well.

@tensionhead
Copy link
Contributor

Ok then I'll put it into our 'nice-to-have eventually' project ;)

@tensionhead tensionhead added this to To do in Codebase Improvements via automation Mar 10, 2022
@tensionhead tensionhead moved this from To do to Done in Codebase Improvements Mar 28, 2022
@tensionhead tensionhead moved this from Done to To do in Codebase Improvements Mar 28, 2022
@tensionhead
Copy link
Contributor

tensionhead commented Jun 14, 2022

Or what about returning the Selector object directly? Such that the 'injections' are working with and only with it:

What we have

data.selectdata(toilim=[0, 1], in_place=True)
# access selected times of 1st trial
data.time[0][data.selection.time[0]]

Proposal

my_selection = data.selectdata(toilim=[0, 1], in_place=True)
# access selected times of 1st trial
my_selection.time[0]

I think that could be a nice interface, at least for our own post-processing like plotting, what u think @pantaray?

EDIT: We also have

# this is a `Selector` object
selector = data.selection
# however setting does not work
data.selection = selector
>>> AttributeError: 'Selector' object has no attribute 'keys'

For things like storing/reverting selections an explicit setter which accepts Selector objects could be useful?!

@pantaray
Copy link
Member Author

I'm not sure I fully understand the proposal. Doing something like data.selectdata(toilim=[0, 1], in_place=True) automagically attaches the Selector instance created in the background to data.selection. I'm not sure why we would want to first create a Selector instance and then attach it.
If I understand correctly, the returned my_selection.time[0] is equivalent to data.time[data.selection.time[0]] right? If yes, and we return the selection directly, we might end up flooding the memory (which would be particularly devious for inplace=True selections ;) )

@tensionhead
Copy link
Contributor

tensionhead commented Sep 6, 2022

So storing and reverting selections now got addressed by simply memorizing the initial select dictionary as a Selector instance attribute. With this things like:

# now modify and re-apply selection                                
select = data.selection.select.copy()                              
select['trials'] = fit_trl_idx                                     
data.selectdata(select, inplace=True)

work without any known problems.

The next step me and @dfsp-spirit discussed is the obj.selection.trials property. This breaks symmetry to the orginal obj.propertys compared to (all) other Selector properties (e.g. trialdefinition, channel, freq, ...). obj.selection.trials returns a list of trial indices, whereas obj.trials returns an Indexer, giving access to the underlying trial arrays directly. We would now try to restore symmetry by creating an Indexer also for the Selctor.trials, with the help of _preview_trial. However, "fancy indexing" is out of the question for the moment. The current Selector.trials will be renamed to sth more descriptive like Selector.trialidx.

EDIT:
Due to necessities these changes will be incorporated into the psth_tests branch

@tensionhead tensionhead moved this from Under Consideration to In progress in Codebase Improvements Sep 6, 2022
@tensionhead tensionhead assigned tensionhead and unassigned pantaray Sep 6, 2022
tensionhead added a commit that referenced this issue Sep 6, 2022
- preparation for plugging an Indexer into Selector.trials
- might be a good idea to also have a `trial_ids` property in base_data actually
- addresses #175
tensionhead added a commit that referenced this issue Sep 6, 2022
- Indexer indices are relative to the selected trials
- not sure if the Indexer itself works as really expected
- addresses #175

Changes to be committed:
	modified:   syncopy/datatype/base_data.py
@tensionhead tensionhead mentioned this issue Sep 6, 2022
15 tasks
@tensionhead
Copy link
Contributor

With the new selection.trials interface we made a huge step forward, yet I still see the merit of being able to directly access the say freq axis of an inplace selection with my_selection.freq. For the user, the only way to achieve that atm is to use a copy (inplace=False) selection. For plotting this got internally solved by little gymnastics like:

idx = dataobject.selection.freq                                                                                                                    
# index selection, only one `freq` for all trials                                                                                                  
freq = dataobject.freq[idx] 

This is not suitable for the public API of course.

@tensionhead tensionhead added the More info needed More information is needed from the person who reported this. label Jan 2, 2023
@dfsp-spirit
Copy link
Collaborator

We should consider deprecating public in-place selections (but keep them in the dev API, of course). Currently we do not find a strong argument to keep them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Explore Examine novel functionality/proposed changes etc. Does not necessarily involve coding things. More info needed More information is needed from the person who reported this.
Projects
Development

No branches or pull requests

3 participants