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

Added mask to SSIM function call #685

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

FelixSteinbauer
Copy link
Contributor

I think the mask should also be provided to the SSIM function call. As the mask variable is always present/valid (either a proper mask if provided by the user or all-ones, see variable initialisation), it should be fine to simply add the mask as parameter to the call.

In my opinion, structural_similarity_index() should work fine with this additional parameter and nothing should break. However, I did not test the suggested code change.

This really is just a very minor change so please don't be mad at me for not following the standard PR procedure UwU

Proposed Changes

  • Set "mask" as optional parameter to structural_similarity_index() for the "synthesis" metric computation case.

Checklist

  • I have read the CONTRIBUTING guide.
  • My PR is based from the current GaNDLF master .
  • Non-breaking change (does not break existing functionality): provide as many details as possible for any breaking change.
  • Function/class source code documentation added/updated.
  • Code has been blacked for style consistency.
  • If applicable, version information has been updated in GANDLF/version.py.
  • Tests added or modified to cover the changes; if coverage is reduced, please give explanation.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@Geeks-Sid
Copy link
Collaborator

@FelixSteinbauer Thanks for your first contribution to GaNDLF. I request you sign the CLA provided by the @CLA Bot before we approve your PR. :)

@FelixSteinbauer
Copy link
Contributor Author

I did fill out the google form a few hours ago. I got a confirmation mail regarding my google form submission. Should I have gotten another/additional email?

Copy link
Collaborator

@sarthakpati sarthakpati left a comment

Choose a reason for hiding this comment

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

LGTM

@sarthakpati
Copy link
Collaborator

I did fill out the google form a few hours ago. I got a confirmation mail regarding my google form submission. Should I have gotten another/additional email?

No need. Once it gets recorded in the system, it will auto-propagate. 😄

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #685 (29e4560) into master (0738c92) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #685   +/-   ##
=======================================
  Coverage   94.62%   94.62%           
=======================================
  Files         116      116           
  Lines        8054     8054           
=======================================
  Hits         7621     7621           
  Misses        433      433           
Flag Coverage Δ
unittests 94.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
GANDLF/cli/generate_metrics.py 100.00% <ø> (ø)

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

@FelixSteinbauer
Copy link
Contributor Author

As soon as I find a printer and scanner, the cla-check will also pass 👍 (probably tomorrow)

@sarthakpati sarthakpati merged commit fdb73f8 into mlcommons:master Jul 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants