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

Restructure blobfinder code for use independent of LiberTEM #14

Merged
merged 11 commits into from
Feb 11, 2020

Conversation

uellue
Copy link
Member

@uellue uellue commented Feb 10, 2020

Closes #6

  • Module base for code that only depends on external projects
  • Module common for code that depends on other LiberTEM common packages, but no core packages.
  • Module udf for code that depends on LiberTEM core facilities

The intention is to allow code re-use without having to install all of LiberTEM, but also with
a reduced number of PyPI packages. Dependencies on LiberTEM common and core will be optional
features.

Still TODO:

  • Modify setup.py accordingly
  • Update documentation

Contributor Checklist:

* Module base for code that only depends on external projects
* Module common for code that depends on other LiberTEM common packages, but no core packages.
* Module udf for code that depends on LiberTEM core facilities

The intention is to allow code re-use without having to install all of LiberTEM, but also with
a reduced number of PyPI packages. Dependencies on LiberTEM common and core will be optional
features.

Still TODO:
* Modify setup.py accordingly
* Update documentation
@uellue uellue added the WIP label Feb 10, 2020
@uellue
Copy link
Member Author

uellue commented Feb 10, 2020

@sk1p could you review the structure before I get to work with setup.py and documentation? :-)

@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #14 into master will increase coverage by 6.37%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   88.72%   95.09%   +6.37%     
==========================================
  Files           7        9       +2     
  Lines         470      469       -1     
  Branches       36       38       +2     
==========================================
+ Hits          417      446      +29     
+ Misses         49       17      -32     
- Partials        4        6       +2
Impacted Files Coverage Δ
src/libertem_blobfinder/udf/utils.py 83.87% <ø> (ø)
src/libertem_blobfinder/__init__.py 100% <100%> (ø) ⬆️
src/libertem_blobfinder/udf/refinement.py 93.54% <100%> (ø)
src/libertem_blobfinder/common/patterns.py 97.64% <100%> (ø)
src/libertem_blobfinder/udf/correlation.py 100% <100%> (ø)
src/libertem_blobfinder/common/correlation.py 100% <100%> (ø)
src/libertem_blobfinder/base/correlation.py 97.65% <46.09%> (ø)

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 06f6df1...f8a3a88. Read the comment docs.

Copy link
Member

@sk1p sk1p left a comment

Choose a reason for hiding this comment

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

General structure 👍

Some comments inline, most importantly about the re-exporting of sub-modules.

src/libertem_blobfinder/base/correlation.py Outdated Show resolved Hide resolved
src/libertem_blobfinder/base/__init__.py Outdated Show resolved Hide resolved
src/libertem_blobfinder/base/correlation.py Show resolved Hide resolved
src/libertem_blobfinder/common/__init__.py Outdated Show resolved Hide resolved
src/libertem_blobfinder/udf/__init__.py Outdated Show resolved Hide resolved
src/libertem_blobfinder/common/correlation.py Outdated Show resolved Hide resolved
@sk1p
Copy link
Member

sk1p commented Feb 10, 2020

Oh, almost forgot: maybe the tests folder can be structured to mirror the code structure? I.e. have tests/test_{base,common,udf}.py?

Feedback @sk1p

FIXME pytest --doctest-modules src/libertem-blobfinder doesn't like the duplicate correlation.py name
To make sure the folders are packages, which means files with the same name
in separate folders are identified as different.
* Test case for visualize_frame
* Split in several test files
* Docstrings
* Imports

Closes LiberTEM#10
@uellue
Copy link
Member Author

uellue commented Feb 10, 2020

Oh, almost forgot: maybe the tests folder can be structured to mirror the code structure? I.e. have tests/test_{base,common,udf}.py?

Done! We just have to keep in mind that the UDF and common parts are responsible for most of the coverage of base.

Thx for your review! :-) Tomorrow I'll start sorting out setup.py and the documentation.

* Move featurevector to common.patterns since it is closely related to them
* Move visualize_frame to UDF folder since it depends on LiberTEM core
* Complete list of imports not required since they are imported explicitly in
  examples now
* Use UDF instead of job in test
* Extras "common" and "udf"
* Some clean-up
setup.py Outdated Show resolved Hide resolved
@uellue
Copy link
Member Author

uellue commented Feb 11, 2020

Perhaps I'll open a separate PR for the documentation to keep the process manageable.

@uellue uellue removed the WIP label Feb 11, 2020
@uellue uellue requested a review from sk1p February 11, 2020 12:14
Copy link
Member

@sk1p sk1p left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍 Most of the codeclimate issues were there before, and should be worked on bit by bit, I think.

@sk1p
Copy link
Member

sk1p commented Feb 11, 2020

Btw. not sure why codecov is complaining, except for the fft fallback to np I don't see any uncovered lines in the diff... 🤷‍♂️

@sk1p sk1p merged commit cdd2173 into LiberTEM:master Feb 11, 2020
@sk1p sk1p added this to the 0.4 milestone Feb 11, 2020
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.

Code structure
2 participants