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

Cleanup of main and optimizer initializations #98

Merged
merged 22 commits into from
May 27, 2021
Merged

Cleanup of main and optimizer initializations #98

merged 22 commits into from
May 27, 2021

Conversation

nwittler
Copy link
Collaborator

What
General cleanup of the script based infrastructure.

Why
There was a lot of baggage from rigid old scripts. To make the software as a whole more extendable and usable, this needed to be cleaned up.

How
Moved relevant code from c3.utils.parsers.py to the respective initialization methods or c3.main.py

Open Questions

  • In calibration (C2) and model learning (C3) the configuration files and therefore the interface should be discussed
  • In particular the handling of C2 communication with the experiment (simulated or real)

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request introduces 4 alerts and fixes 9 when merging b833c13 into 6e7e7a0 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 2 for Modification of parameter with default

fixed alerts:

  • 6 for Unused local variable
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Unused import

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2021

Codecov Report

Merging #98 (13292a4) into dev (8e747d5) will increase coverage by 5.83%.
The diff coverage is 60.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #98      +/-   ##
==========================================
+ Coverage   51.48%   57.32%   +5.83%     
==========================================
  Files          36       35       -1     
  Lines        5409     5279     -130     
==========================================
+ Hits         2785     3026     +241     
+ Misses       2624     2253     -371     
Impacted Files Coverage Δ
c3/main.py 55.17% <56.41%> (+55.17%) ⬆️
c3/optimizers/c1.py 72.72% <58.82%> (-8.44%) ⬇️
c3/optimizers/c3.py 19.47% <59.37%> (+19.47%) ⬆️
c3/optimizers/sensitivity.py 30.96% <59.37%> (+30.96%) ⬆️
c3/optimizers/c2.py 32.30% <66.66%> (+32.30%) ⬆️
c3/utils/utils.py 65.04% <80.00%> (+0.40%) ⬆️
c3/experiment.py 62.14% <100.00%> (+3.50%) ⬆️
c3/qiskit/c3_backend.py 78.80% <100.00%> (ø)
c3/system/chip.py
c3/system/model.py
... and 14 more

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 8e747d5...13292a4. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request fixes 9 alerts when merging dce7007 into 6e7e7a0 - view on LGTM.com

fixed alerts:

  • 6 for Unused local variable
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Unused import

lazyoracle
lazyoracle previously approved these changes May 26, 2021
Copy link
Member

@lazyoracle lazyoracle left a comment

Choose a reason for hiding this comment

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

Ready to merge! 🚀

c3/main.py Outdated
opt.set_exp(exp)
opt.set_created_by(opt_config)
opt.set_created_by(cfg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be the location of the config file not the cfg dict.

fedroy
fedroy previously approved these changes May 27, 2021
Copy link
Collaborator

@fedroy fedroy left a comment

Choose a reason for hiding this comment

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

Just went thorugh it with @nwittler, up to hotfix of the storing the right config in main.py, the code is good for merging.

@nwittler nwittler dismissed stale reviews from fedroy and lazyoracle via 13292a4 May 27, 2021 15:06
Copy link
Member

@lazyoracle lazyoracle left a comment

Choose a reason for hiding this comment

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

:shipit:

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request fixes 9 alerts when merging 13292a4 into e10518e - view on LGTM.com

fixed alerts:

  • 6 for Unused local variable
  • 2 for Wrong name for an argument in a class instantiation
  • 1 for Unused import

@nwittler nwittler merged commit b6dd98f into dev May 27, 2021
@nwittler nwittler deleted the fix-parsers branch May 27, 2021 15:54
@lazyoracle lazyoracle added this to the 1.3 milestone May 28, 2021
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.

Undefined references in parsers.py Incorrect intantiation of classes
4 participants