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

example quantum anomaly detection #714

Merged
merged 31 commits into from
Dec 14, 2022
Merged

Conversation

Simone-Bordoni
Copy link
Contributor

@Simone-Bordoni Simone-Bordoni commented Nov 28, 2022

To be completed,
I will add README, tests and .ipynb

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (f57cf61) compared to base (2606a97).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #714   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           88        88           
  Lines        11629     11629           
=========================================
  Hits         11629     11629           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@scarrazza scarrazza added the enhancement New feature or request label Nov 28, 2022
@scarrazza scarrazza marked this pull request as draft November 29, 2022 19:24
@scarrazza scarrazza added this to the Qibo next milestone Nov 29, 2022
@Simone-Bordoni Simone-Bordoni force-pushed the anomaly_detection_example branch from d5873bf to a7bd257 Compare December 5, 2022 11:02
@Simone-Bordoni
Copy link
Contributor Author

I need some help with tests that are not passing.
When the script is run locally everythong works.

@scarrazza
Copy link
Member

From the last crash, seems like you are writing to a folder that does not exist: results/loss_distribution.png.

@Simone-Bordoni Simone-Bordoni marked this pull request as ready for review December 5, 2022 16:13
Copy link
Contributor

@MatteoRobbiati MatteoRobbiati 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 this, it is in a good shape for me.
I left in the review some small comments, many of which are really small fixes. I suggest two larger changes:

  1. Reverse the location of the "How to run an example" section with the theoretical section in README.md. I think it's more natural to read the theoretical background and then have in sequence: explanation of how to execute, anticipation of the results to be derived.
  2. Add docstrings in the codes so that the implemented functions are clear when reading.

examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
@Simone-Bordoni Simone-Bordoni force-pushed the anomaly_detection_example branch from 64bff4a to 65c23ee Compare December 12, 2022 11:10
@scarrazza
Copy link
Member

@MatteoRobbiati could you please have another look?

Copy link
Contributor

@MatteoRobbiati MatteoRobbiati 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 your work.

Some more suggestions follow.
If you like them, you can apply the modifications directly by adding the suggestions I purpose (add sugg on batch and then use the Commit suggestion button).

examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/README.md Outdated Show resolved Hide resolved
examples/anomaly_detection/test.py Outdated Show resolved Hide resolved
examples/anomaly_detection/test.py Outdated Show resolved Hide resolved
examples/anomaly_detection/train.py Outdated Show resolved Hide resolved
examples/anomaly_detection/train.py Outdated Show resolved Hide resolved
examples/anomaly_detection/train.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MatteoRobbiati MatteoRobbiati 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 the last modifications.
It is ok for me.

@MatteoRobbiati
Copy link
Contributor

I have done a last small fix, now I think it can be merged.

Copy link
Member

@scarrazza scarrazza 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 have linked the new example in sphinx.

@scarrazza scarrazza merged commit bd13a18 into master Dec 14, 2022
@scarrazza scarrazza deleted the anomaly_detection_example branch January 23, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants