QuTech-Delft / OpenQL

OpenQL: A Portable Quantum Programming Framework for Quantum Accelerators. https://dl.acm.org/doi/10.1145/3474222
https://openql.readthedocs.io
Other
99 stars 44 forks source link

make bundles start at cycle 0 instead of 1 #398

Closed wvlothuizen closed 3 years ago

jvanstraten commented 3 years ago

This is a Bad Idea™ because this influences things all over the place in subtle ways. For instance, I happen to know that the cQASM reader also assumes that cycles start at 1. While Hans and I happen to agree with you that not starting at 0 is silly, you can't just essentially change the IR because you don't agree with what's essentially a matter of taste without thoroughly reviewing all of OpenQL, its documentation, the other readers and writers etc. to see what depends on this, and that just isn't worth the effort when everything else in the IR is equally messy.

I find it pretty concerning that CI is not screaming at you for making this change, by the way. I'm not necessarily surprised (considering OpenQL only has integration tests) but apparently code coverage is pretty bad.

wvlothuizen commented 3 years ago

It hardly looks like an explicit choice that numbering started at 1, because it is basically an implicit consequence of the duration of the SOUCE/SINK pseudo gates. And note that that the comments in ir::bundler() suggest that the bundle starts at zero, but it doesn't because the first gate starts at 1.

I would personally prefer to move forward and solve this for real, but otherwise the least we should do is to add a constant that makes the choice explicit. Then I can also safely update the CC backend which currently adds a senseless 20 ns pause in front of every kernel, which isn't a problem when an experiment constitutes a single kernel, but is a waste if it isn't.

jvansomeren commented 3 years ago

I also found a place in src/mapper.cchttp://mapper.cc that relates to starting at 1. So we have src/ir.cchttp://ir.cc, src/scheduler.cchttp://scheduler.cc, src/gate.cchttp://gate.cc and src/mapper.cchttp://mapper.cc to be updated at least.

We can go for schizophrenically starting at 1 with SOURCE/SINK taking 1 cycle and then shifting back to 0 later, or start with SOURCE/SINK taking 0 cycles and updating the other parts consistently. I prefer the latter one, but that needs careful changing in a sandbox.

And still I’m not sure that I’ve seen all places.

Best,

Hans

Op 24 mrt. 2021, om 16:42 heeft Wouter Vlothuizen @.**@.>> het volgende geschreven:

It hardly looks like an explicit choice that numbering started at 1, because it is basically an implicit consequence of the duration of the SOUCE/SINK pseudo gates. And note that that the comments in ir::bundler() suggest that the bundle starts at zero, but it doesn't because the first gate starts at 1.

I would personally prefer to move forward and solve this for real, but otherwise the least we should do is to add a constant that makes the choice explicit. Then I can also safely update the CC backend which currently adds a senseless 20 ns pause in front of every kernel, which isn't a problem when an experiment constitutes a single kernel, but is a waste if it isn't.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_QE-2DLab_OpenQL_pull_398-23issuecomment-2D805934100&d=DwMCaQ&c=XYzUhXBD2cD-CornpT4QE19xOJBbRy-TBPLK0X9U2o8&r=kNdT9ewT6pQdYFkBLR_5-ZqsrSTk7k5Hdd7MSC_Vnzg&m=MbT10TfR0ZYcDQqLV_1v6vgMZhs4FQZOEW1t7mxck2I&s=3gN7ldczUVRvhnsT7bvFDvNqB6M_0dLDn2XguqyldMI&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AEDTBNWIICSFJUJZDJ6FTPDTFICAFANCNFSM4ZXEWWVQ&d=DwMCaQ&c=XYzUhXBD2cD-CornpT4QE19xOJBbRy-TBPLK0X9U2o8&r=kNdT9ewT6pQdYFkBLR_5-ZqsrSTk7k5Hdd7MSC_Vnzg&m=MbT10TfR0ZYcDQqLV_1v6vgMZhs4FQZOEW1t7mxck2I&s=1zL5Z8BayRmR6q57dY0hjh4RwII4C6JsaHSeQkoOQPE&e=.

jvansomeren commented 3 years ago

And furthermore src/metrics.cchttp://metrics.cc and another place in src/scheduler.cchttp://scheduler.cc, both probably not used currently.

I’m wondering about disturbing messages of starting at 1 and starting from 0 in the same source base. This requires in-depth analysis. It hopefully has to do with having the depgraph with SOURCE/SINK added around a circuit and circuits themselves without SOURCE/SINK.

Op 24 mrt. 2021, om 17:13 heeft Hans van Someren - EWI @.**@.>> het volgende geschreven:

I also found a place in src/mapper.cchttp://mapper.cc/ that relates to starting at 1. So we have src/ir.cchttp://ir.cc/, src/scheduler.cchttp://scheduler.cc/, src/gate.cchttp://gate.cc/ and src/mapper.cchttp://mapper.cc/ to be updated at least.

We can go for schizophrenically starting at 1 with SOURCE/SINK taking 1 cycle and then shifting back to 0 later, or start with SOURCE/SINK taking 0 cycles and updating the other parts consistently. I prefer the latter one, but that needs careful changing in a sandbox.

And still I’m not sure that I’ve seen all places.

Best,

Hans

Op 24 mrt. 2021, om 16:42 heeft Wouter Vlothuizen @.**@.>> het volgende geschreven:

It hardly looks like an explicit choice that numbering started at 1, because it is basically an implicit consequence of the duration of the SOUCE/SINK pseudo gates. And note that that the comments in ir::bundler() suggest that the bundle starts at zero, but it doesn't because the first gate starts at 1.

I would personally prefer to move forward and solve this for real, but otherwise the least we should do is to add a constant that makes the choice explicit. Then I can also safely update the CC backend which currently adds a senseless 20 ns pause in front of every kernel, which isn't a problem when an experiment constitutes a single kernel, but is a waste if it isn't.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_QE-2DLab_OpenQL_pull_398-23issuecomment-2D805934100&d=DwMCaQ&c=XYzUhXBD2cD-CornpT4QE19xOJBbRy-TBPLK0X9U2o8&r=kNdT9ewT6pQdYFkBLR_5-ZqsrSTk7k5Hdd7MSC_Vnzg&m=MbT10TfR0ZYcDQqLV_1v6vgMZhs4FQZOEW1t7mxck2I&s=3gN7ldczUVRvhnsT7bvFDvNqB6M_0dLDn2XguqyldMI&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AEDTBNWIICSFJUJZDJ6FTPDTFICAFANCNFSM4ZXEWWVQ&d=DwMCaQ&c=XYzUhXBD2cD-CornpT4QE19xOJBbRy-TBPLK0X9U2o8&r=kNdT9ewT6pQdYFkBLR_5-ZqsrSTk7k5Hdd7MSC_Vnzg&m=MbT10TfR0ZYcDQqLV_1v6vgMZhs4FQZOEW1t7mxck2I&s=1zL5Z8BayRmR6q57dY0hjh4RwII4C6JsaHSeQkoOQPE&e=.

jvanstraten commented 3 years ago

So are we actually doing this now then or what? I'm confused.

jvansomeren commented 3 years ago

I propose to discuss this tomorrow in the meeting. It takes time that we don’t have or takes time away from other efforts. Our current, probably more urgent efforts are:

  1. getting the modularization interfaces stable
  2. demo of visualizer in this new modularization context
  3. cQASM 2.0 language and design of implementation

So if it is just 1 cycle, this is not nice, but is not urgent.

Op 24 mrt. 2021, om 18:20 heeft jvanstraten @.**@.>> het volgende geschreven:

So are we actually doing this now then or what? I'm confused.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_QE-2DLab_OpenQL_pull_398-23issuecomment-2D806011170&d=DwMCaQ&c=XYzUhXBD2cD-CornpT4QE19xOJBbRy-TBPLK0X9U2o8&r=kNdT9ewT6pQdYFkBLR_5-ZqsrSTk7k5Hdd7MSC_Vnzg&m=A-VVic0I6_se708wO7Din35o0l06mZM2Wi93-P2zmWg&s=6JcdUdyoyASq17drq5ZhDhmOFrzxi7qx4M4xoXFj0Tc&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AEDTBNVD56E5B35TCQO3TBDTFINNFANCNFSM4ZXEWWVQ&d=DwMCaQ&c=XYzUhXBD2cD-CornpT4QE19xOJBbRy-TBPLK0X9U2o8&r=kNdT9ewT6pQdYFkBLR_5-ZqsrSTk7k5Hdd7MSC_Vnzg&m=A-VVic0I6_se708wO7Din35o0l06mZM2Wi93-P2zmWg&s=89MNskTgdDoA8pI2c_nefRTFrwKAnMrzS3su_pWlBbA&e=.