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

Refactoring #46

Merged
merged 13 commits into from
Feb 14, 2022
Merged

Refactoring #46

merged 13 commits into from
Feb 14, 2022

Conversation

thisFreya
Copy link
Collaborator

Refactored according to Nina's requests - reorganized file structure and renamed many functions accordingly.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #46 (ee100db) into master (1bc504f) will decrease coverage by 1.50%.
The diff coverage is 97.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   95.32%   93.83%   -1.49%     
==========================================
  Files           7        6       -1     
  Lines         363      259     -104     
==========================================
- Hits          346      243     -103     
+ Misses         17       16       -1     
Impacted Files Coverage Δ
ioSPI/micrographs.py 96.43% <96.43%> (ø)
ioSPI/particle_metadata.py 97.23% <97.23%> (ø)
ioSPI/atomic_models.py 98.22% <100.00%> (ø)
ioSPI/fourier.py 98.51% <0.00%> (-1.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bc504f...ee100db. Read the comment docs.

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

First comment, will finish the review later :) thanks!

ioSPI/micrographs.py Outdated Show resolved Hide resolved
Copy link
Contributor

@geoffwoollard geoffwoollard left a comment

Choose a reason for hiding this comment

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

overall looks good. files and functions more specific and informative.

@geoffwoollard geoffwoollard self-requested a review February 10, 2022 05:03
Copy link
Contributor

@geoffwoollard geoffwoollard left a comment

Choose a reason for hiding this comment

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

resolve that linting error. I'm fine merging when Nina is.

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Very nice, thank you!

BTW: are you using pycharm or vscode with the functionality that allows refactoring quickly? E.g. vscode has an option "change all occurences", which is convention for this (see screenshot).

It is looking much better, thank you very much for your patience in this!

I have made some comments on the conventions regarding the names of the input parameters in the functions now (which is in the same spirit of my previous remarks on the file names and function names, now we're looking more granularly at the parameters' names).

The parameter names should also follow a consistent convention (otherwise the user gets crazy because they needs to remember that write_atomic_model_* takes output_path as first parameter, but write_micrograph_* takes path as first parameter and write_metadata_* takes path as second parameter : not a great experience.

I have made comments along these lines, but I'll let you go through the files to see if, in this spirit, everything can be made rigorously consistent.

Thanks!

Screen Shot 2022-02-10 at 7 40 51 AM

ioSPI/atomic_models.py Outdated Show resolved Hide resolved
ioSPI/micrographs.py Outdated Show resolved Hide resolved
ioSPI/micrographs.py Outdated Show resolved Hide resolved
ioSPI/micrographs.py Outdated Show resolved Hide resolved
ioSPI/micrographs.py Outdated Show resolved Hide resolved
ioSPI/particle_metadata.py Outdated Show resolved Hide resolved
ioSPI/particle_metadata.py Outdated Show resolved Hide resolved
@ninamiolane
Copy link
Contributor

And yes, please fix the linting error. It might be coming from an update in black, it has been popping up on other projects that I manage.

It could be fixed by running black . in the root of this repo.

@thisFreya
Copy link
Collaborator Author

thisFreya commented Feb 10, 2022

@ninamiolane Changes implemented, please let me know if there's anything else that can be better represented!

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the changes.

@ninamiolane ninamiolane merged commit d39e1cd into master Feb 14, 2022
@ninamiolane ninamiolane mentioned this pull request Feb 14, 2022
@fredericpoitevin fredericpoitevin deleted the refactoring branch March 25, 2022 01:53
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