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

Change name of asel in timeseries to something more comprehensible. #3911

Open
hmacdope opened this issue Nov 6, 2022 · 12 comments · Fixed by #4343
Open

Change name of asel in timeseries to something more comprehensible. #3911

hmacdope opened this issue Nov 6, 2022 · 12 comments · Fixed by #4343
Assignees
Labels
Component-Readers deprecation Deprecated functionality to give advance warning for API changes. remove-3.0 deprecated and remove in release 3.0 usability
Milestone

Comments

@hmacdope
Copy link
Member

hmacdope commented Nov 6, 2022

    I know we're just doing the same as what memoryreader does, but it just struck me that `asel` is such a weird parameter name for an atomgroup. Not much we can do here, but if I'm not the only one maybe we can consider changing this for 3.0 (in a different PR, let's get this merged!).

Originally posted by @IAlibay in #3890 (comment)

I agree with @IAlibay that the name asel is a little confusing and could do with a change in 3.0.
leave your opinions below.

Will require a deprecation warning first if we go ahead.

@hmacdope hmacdope self-assigned this Nov 6, 2022
@hmacdope hmacdope added this to the Release 3.0 milestone Nov 6, 2022
@orbeckst
Copy link
Member

orbeckst commented Nov 11, 2022

yes, please, asel ▶️ selection ? EDIT: atomgroup (see below)

@IAlibay
Copy link
Member

IAlibay commented Nov 11, 2022

yes, please, asel ▶️ selection ?

asel is an atomgroup right? atomgroup might be closer to what we do elsewhere?

@orbeckst
Copy link
Member

yeah, you're right

@orbeckst orbeckst added usability Component-Readers deprecation Deprecated functionality to give advance warning for API changes. labels Jan 3, 2023
@hmacdope
Copy link
Member Author

@MDAnalysis/gsoc-mentors making this a gsoc-starter issue

@IAlibay
Copy link
Member

IAlibay commented Feb 14, 2023

@hmacdope isn't this a 3.0 targeted change? We wouldn't be able to merge it during the application period.

@hmacdope
Copy link
Member Author

Ah yes sorry I meant the deprecation warning part. Do we think that is suitable ?

@IAlibay
Copy link
Member

IAlibay commented Feb 14, 2023

Ah yes sorry I meant the deprecation warning part. Do we think that is suitable ?

Sounds good to me!

@tushdemort
Copy link

hi @hmacdope, I want to work on MDAnalysis as part of GSoC and wanted to take up this issue. Seems like it won't get merged during the application period. However, can I work on it?

@hmacdope
Copy link
Member Author

hmacdope commented Mar 9, 2023

Please go ahead and make a PR! If you make a fix it can be merged in the application period.

@tushdemort
Copy link

Hi @hmacdope

I have made changes for asel in timeseries wherever the function is defined and called. I have also made changes in the test code for the same. Do you think sample_atom_group is a good variable name in place of asel (can be a little wordy)? atom_group and atomgroup are used as variable/function names in a couple of files.

Also, I didn't understand the discussion regarding the deprecation warning, can you explain it a bit?

@IAlibay
Copy link
Member

IAlibay commented Dec 3, 2023

@hmacdope I'm reopening to keep an issue to track the removal of asel - please do reclose if there's another one already.

@hmacdope
Copy link
Member Author

hmacdope commented Dec 3, 2023

Cheers, nah this is it.

@RMeli RMeli added remove-3.0 deprecated and remove in release 3.0 and removed GSOC Starter labels Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component-Readers deprecation Deprecated functionality to give advance warning for API changes. remove-3.0 deprecated and remove in release 3.0 usability
Projects
None yet
5 participants