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

F811, F523/F524, F841, F741 - Corrections in crystal.py #571

Closed

Conversation

alex-rakowski
Copy link
Collaborator

@alex-rakowski alex-rakowski commented Nov 16, 2023

F811, F523/F524, F841, F741 - Corrections

@alex-rakowski
Copy link
Collaborator Author

added the changes to linter.yaml, .flake8 and removed build-flake.yaml as the GA is in a strange state in dev at the moment, so the test would fail.

Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

The changes look good, you can delete those two unused variables in crystal.py and then I think this is ready to go.

py4DSTEM/process/diffraction/crystal.py Outdated Show resolved Hide resolved
py4DSTEM/process/diffraction/crystal.py Outdated Show resolved Hide resolved
@sezelt
Copy link
Member

sezelt commented Nov 17, 2023

This does seem to overlap with #563 a bit, so maybe they should be combined? Probably the only potential conflict is if they make different changes to the flake8 file

@sezelt
Copy link
Member

sezelt commented Nov 19, 2023

I'm approving this since all the changes it makes are acceptable, but I'll hold off on merging so that we can decide how to reconcile with #563

Copy link
Member

@bsavitzky bsavitzky left a comment

Choose a reason for hiding this comment

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

See comment in-thread

@bsavitzky
Copy link
Member

Hey @alex-rakowski -

This PR is redundant / in conflict with PR#563. All file lines changed here other than those in crystal.py have already been modified, often quite differently, in that PR. Can we sync up?

Ultimately this should be a single PR, otherwise we're just making more work for ourselves!

@sezelt
Copy link
Member

sezelt commented Mar 8, 2024

I've merged this with #560

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