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

Reorganisation of utils #996

Closed
pc494 opened this issue Jan 27, 2024 · 7 comments · Fixed by #1055
Closed

Reorganisation of utils #996

pc494 opened this issue Jan 27, 2024 · 7 comments · Fixed by #1055

Comments

@pc494
Copy link
Member

pc494 commented Jan 27, 2024

So I got started on the utils folder in #994 and I think I've got a handle on what we have. Broadly utils modules fall into three categories.

Internal use only

e.g. _deprecated, _slicer

Functionality that the devs need but that we either don't expect or don't want end users to make use of. My plan here is to simply be more explicit with these modules (i.e making the private modules, rather than just private functions). For example I don't think we want our users trying to use our cuda code themselves . So cuda_utils would be moved to a private module (with required deprecations etc).

Support for niche functionality

We originally adopted a utils-heavy pattern. Many classes have supporting functions (that generally get shoved in map) that have their own util folders. I would consider it better to just move such functionality directly into the module that defines the class. This (for example) is what I've done in #994 for the utils that support the ReducedIntensity classes.

True, user-facing utils

An obvious example of this group is plotting_utils. We want users to use them, they are utils. I don't plan to change much about these except to rename modules to drop the trailing `util'. I think it's uncontentious to suggest that:

from pyxem.utils.plotting_utils import *

is weird when you could have

from pyxem.utils.plotting import *

with a deprecation warning on the old imports.

@pc494
Copy link
Member Author

pc494 commented Feb 20, 2024

#1020 didn't update the changelog. Will sweep this up when I do the next batch of utils.

Done.

@pc494 pc494 self-assigned this Feb 20, 2024
@pc494
Copy link
Member Author

pc494 commented Mar 12, 2024

What's left?

No action at this time.

ransac_ellipse_tools.py
polar_transform_utils.py see #1015
indexation_utils.py

@magnunor
Copy link
Collaborator

I should really look through some of the old pixstem ones, as there are probably several which need some tidying.

But I'm not sure exactly when I'll have the time.

@pc494
Copy link
Member Author

pc494 commented Mar 17, 2024

So with #1032, #1033, #1034 and #1035 I've got this down to 8 that need addressing. If they get technical I'll raise separate issues from now on.

@CSSFrancis
Copy link
Member

@pc494 I don't know if we are going to get much farther with this. I'm not sure that the signals should be private. There are too many instances where we want to directly create a signal that I think they should be public.

@pc494
Copy link
Member Author

pc494 commented Mar 27, 2024

@CSSFrancis
Copy link
Member

As in the functions in https://github.com/pyxem/pyxem/blob/main/pyxem/utils/signal.py ?

I completely forgot we have a utils/signals. Yes that Should be private. I was thinking you were trying to make the signals module private and I was a bit confused.

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

Successfully merging a pull request may close this issue.

3 participants