Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Additional affines #29

Closed
wants to merge 10 commits into from
Closed

Additional affines #29

wants to merge 10 commits into from

Conversation

justusschock
Copy link
Member

@justusschock justusschock commented Jan 3, 2020

Short Description

This PR adds additional Affine Transformations. It mainly includes Transformations around the image center and a resizing trafo.

PR Checklist

PR Implementer

This is a small checklist for the implementation details of this PR.
If you submit a PR, please look at these points (don't worry about the RisingTeam
and Reviewer workflows, the only purpose of those is to have a compact view of
the steps)

If there are any questions regarding code style or other conventions check out our
summary.

  • Implementation
  • Docstrings & Typing
  • Check __all__ sections and __init__
  • Unittests (look at the line coverage of your tests, the goal is 100%!)
  • Update notebooks & documentation if necessary
  • Pass all tests
  • Add the checksum of the last implementation commit to the Changelog

RisingTeam

RisingTeam workflow
  • Add pull request to project (optionally delete corresponding project note)
  • Assign correct label (if you don't have permission to do this, someone will do it for you.
    Please make sure to communicate the current status of the pr.)
  • Does this PR close an Issue? (add closes #IssueNumber at the bottom if
    not already in description)

Reviewer

Reviewer workflow
  • Do all tests pass? (Unittests, NotebookTests, Documentation)
  • Does the implementation follow rising design conventions?
  • Are the tests useful? (!!!) Are additional tests needed?
    Can you think of critical points which should be covered in an additional test?
  • Optional: Check coverage locally / Check tests locally if GPU is necessary to execute

@mibaumgartner mibaumgartner added FeatureRequest Request for additional feature WIP Work in proress labels Jan 3, 2020
@mibaumgartner
Copy link
Member

mibaumgartner commented Jan 10, 2020

I would like to propose a different naming convention: the default behaviour should be the rotation around the image center (consistent to other frameworks) and the current rotation (from the master branch) should be renamed to something differently (e.g. RotationOrigin)

@justusschock
Copy link
Member Author

And same for scaling?

@codecov
Copy link

codecov bot commented Feb 15, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@b1aea08). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #29   +/-   ##
=========================================
  Coverage          ?   98.83%           
=========================================
  Files             ?       34           
  Lines             ?     1292           
  Branches          ?        0           
=========================================
  Hits              ?     1277           
  Misses            ?       15           
  Partials          ?        0
Flag Coverage Δ
#unittests 98.83% <87.5%> (?)
Impacted Files Coverage Δ
rising/utils/affine.py 100% <ø> (ø)
rising/loading/collate.py 100% <100%> (ø)
rising/transforms/affine.py 98.03% <100%> (ø)
rising/transforms/__init__.py 100% <100%> (ø)
rising/transforms/functional/affine.py 100% <100%> (ø)
rising/loading/loader.py 94.38% <85.29%> (ø)

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 b1aea08...abe3757. Read the comment docs.

@justusschock
Copy link
Member Author

Do you want to merge this despite the refactoring PR?

@mibaumgartner
Copy link
Member

We need to undo my commit, rename the transforms, rename OriginMixin to something like CenterMixin and check the tests again
Not sure if we really need the origin transforms though .. but we could keep them if you want

@justusschock justusschock mentioned this pull request Feb 17, 2020
21 tasks
@justusschock
Copy link
Member Author

closed in favour of #31 and #32

@justusschock justusschock deleted the additional_affines branch February 27, 2020 14:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest Request for additional feature WIP Work in proress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants