Skip to content

NEW: Add SCCNN Cell Detection Architecture #434

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 48 commits into from
Sep 2, 2022
Merged

NEW: Add SCCNN Cell Detection Architecture #434

merged 48 commits into from
Sep 2, 2022

Conversation

@shaneahmed
Copy link
Member

@shaneahmed shaneahmed commented Jul 27, 2022

  • Add SCCNN architecture.

@shaneahmed
NEW: Add SCCNN architecture.
627b68c 
- Add SCCNN architecture.
@shaneahmed
DOC: Add docs to SCCNN architecture.
4606025 
- Add docs to SCCNN architecture.
@codecov
Copy link

codecov bot commented Jul 27, 2022 •
edited
Loading

Codecov Report

Merging #434 (3ef7ab3) into develop (1ba4272) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #434      +/-   ##
===========================================
+ Coverage    98.59%   98.62%   +0.02%     
===========================================
  Files           58       59       +1     
  Lines         5784     5872      +88     
  Branches      1047     1049       +2     
===========================================
+ Hits          5703     5791      +88     
  Misses          69       69              
  Partials        12       12
Impacted Files Coverage Δ
tiatoolbox/models/architecture/micronet.py 100.00% <ø> (ø)
tiatoolbox/models/architecture/sccnn.py 100.00% <100.00%> (ø)
tiatoolbox/models/architecture/unet.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaneahmed
NEW: Add preproc function to sccnn.py.
5b6d58a 
- Add preproc function to sccnn.py.
@shaneahmed
NEW: Add infer_batch function to sccnn.py.
106deec 
- Add infer_batch function to sccnn.py.
@shaneahmed
NEW: Add postproc function to sccnn.py.
290e2ae 
- Add postproc function to sccnn.py.
@John-P John-P added the enhancement New feature or request label Jul 27, 2022
@John-P John-P changed the title NEW: Add SCCNN architecture. NEW: Add SCCNN architecture Jul 27, 2022
@John-P John-P changed the title NEW: Add SCCNN architecture NEW: Add SCCNN Architecture Jul 27, 2022
@shaneahmed
TST: Add tests for sccnn.py.
e28252b 
- Add tests for sccnn.py.
@shaneahmed shaneahmed changed the title NEW: Add SCCNN Architecture NEW: Add SCCNN Cell Detection Architecture Jul 27, 2022
@shaneahmed shaneahmed marked this pull request as ready for review July 27, 2022 12:07
@shaneahmed shaneahmed self-assigned this Jul 27, 2022
shaneahmed and others added 11 commits July 27, 2022 15:54
John-P
John-P suggested changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@John-P John-P left a comment

Choose a reason for hiding this comment

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

Thank you for adding this model! Looks good but I think some of the function names, especially for the layers and a bit inconsistent and vague. More verbose but helpful names would improve the readability and maintainability of the code.

@shaneahmed
REF: Refactor functions to meaningful names.
6dcf873 
- Refactor functions to meaningful names.
@shaneahmed
REF: Refactor variables to meaningful names.
0b7bd23 
- Refactor variables to meaningful names.
John-P
John-P reviewed Aug 12, 2022
View reviewed changes
John-P
John-P reviewed Aug 12, 2022
View reviewed changes
John-P
John-P reviewed Aug 12, 2022
View reviewed changes
John-P
John-P suggested changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@John-P John-P 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, just some type hint, docstring, and formatting suggestions.

@shaneahmed
DOC: Adds information for tests in docstring.
906d751 
- Adds information for tests in docstring.
@shaneahmed
DOC: Fixing type hint, docstring, and formatting.
b30c6eb 
- Fixing type hint, docstring, and formatting.
@shaneahmed
BUG: Fix G200 Logging statement uses exception.
5314e10 
- Fix G200 Logging statement uses exception.
@shaneahmed
BUG: Fix Unsupported operand |.
cc45655 
- Fix Unsupported operand |.
@shaneahmed shaneahmed requested a review from John-P August 12, 2022 16:54
shaneahmed
shaneahmed commented Aug 15, 2022
View reviewed changes
@shaneahmed
DOC: Update sccnn-conic
c7facb7 
- Update sccnn-conic
@shaneahmed
DOC: Fix docstring.
33b5ae1 
-  Fix docstring.
@shaneahmed shaneahmed removed the request for review from vqdang August 16, 2022 11:04
@shaneahmed
DOC: Fix sccnn docstring.
84be6c1 
- Fix sccnn docstring.
shaneahmed
shaneahmed commented Aug 17, 2022
View reviewed changes
@shaneahmed
Update tiatoolbox/models/architecture/sccnn.py
fa05564 
Update tiatoolbox/models/architecture/sccnn.py
@shaneahmed shaneahmed requested a review from vqdang August 19, 2022 10:27
John-P
John-P approved these changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@John-P John-P 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. I like Dang's suggestion of moving the spatially constrained layer into its own class (easier to re-use), but otherwise good. Happy either way.

@shaneahmed
Copy link
Member Author

shaneahmed commented Aug 26, 2022

Looks good. I like Dang's suggestion of moving the spatially constrained layer into its own class (easier to re-use), but otherwise good. Happy either way.

I had discussed this with Dang earlier. This meant retraining the model separately. So we decided not to do this.

mostafajahanifar
mostafajahanifar requested changes Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@mostafajahanifar mostafajahanifar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Shan for revising this code. All look good now.

I just add a comment about moving the preprocessing into Forward method (I made the same suggestion for MapDe as well). I'm happy for you to merge the branch after that change :-)

@shaneahmed shaneahmed merged commit e4534d1 into develop Sep 2, 2022
@shaneahmed shaneahmed deleted the feature-sccnn-detection branch September 2, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mostafajahanifar mostafajahanifar mostafajahanifar requested changes

@vqdang vqdang Awaiting requested review from vqdang

+1 more reviewer

@John-P John-P John-P approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@shaneahmed shaneahmed

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shaneahmed @John-P @vqdang @mostafajahanifar