Conversation
|
The helper functions cited in the README.md for energy-time rescaling and embedding are being reviewed here, and should be straightforward (simplified) pull request here once merged in the more complex context: |
dankinn
left a comment
dankinn
left a comment
There was a problem hiding this comment.
Will review this in bursts. A couple of errors, one in testing and one when running app.py.
There was a problem hiding this comment.
Thanks for all your work on this @gunnarmiller!!
Just did a first pass of the UI so far. I'll make a commit to clean up the interface in the next couple days
This is review doesn't include running the demo, just looking at the code right now
dankinn
left a comment
dankinn
left a comment
There was a problem hiding this comment.
Another batch of comments (I'm still working through the whole codebase).
thisac
left a comment
thisac
left a comment
There was a problem hiding this comment.
Thanks for all the hard work @gunnarmiller. I've just had a general look at the code structure. Will do a more thorough review later.
I'm getting quite a few:
Exception: Block 84b95097ef5db8fb19c27b3259e9bd23597a3330acd0a57333d88efe713ad8d8 has predecessor 42ed2371065bf3876890a713c10385b0743c9b0e0263551f847e552573071660 which is not found in the tree.
when running the demo.
Also, the miner table isn't scaling properly when switching miner view.
Very neat demo, but there's some further clean-up required as well as fixing a couple of issues. I can help out with some fixes if needed.
| ) | ||
| def toggle_left_column(collapse_trigger: int, to_collapse_class: str) -> tuple[str, str]: | ||
| """Toggles a 'collapsed' class that hides and shows some aspect of the UI. | ||
| def simulation( |
There was a problem hiding this comment.
This function should be refactored to reduce complexity.
Suggestions:
- The code block under
if len(blockchain_structure) > 0:can be moved into a separate function (restart_trial, perhaps?) - Plotting can be moved into a separate function
- Trial iterations can be moved into a separate function
src/agents/trial_manager.py
Outdated
|
|
||
| # TODO figure out what to do if mining fails | ||
|
|
||
| raise Exception(f"TrialManager exceeded max mining attempts of {self.max_mining_attempts} without {self.mining_miner_id} finding mining a valid block.") |
There was a problem hiding this comment.
Line length too long.
This should raise a more specific Exception
src/utilities/spiral_plotter.py
Outdated
|
|
||
| return traces | ||
|
|
||
| def draw_spiral(self, active_blocks: list[str], active_block_cutoff: int | None = None, mining_block: BlockNode | None = None): |
There was a problem hiding this comment.
Function is quite long - consider refactoring to reduce complexity
jackraymond
left a comment
jackraymond
left a comment
There was a problem hiding this comment.
The changes I am proposing are covered by:
#13
Definitely worth eyeballing the PR differences before merging. I think they shouldn't conflict with other review requests, and resolve several of them.
jackraymond
left a comment
jackraymond
left a comment
There was a problem hiding this comment.
LGTM for purposes of PRA reference
Co-authored-by: Jack Raymond <10591246+jackraymond@users.noreply.github.com> Co-authored-by: Kate Culver <kateeculver@gmail.com>
gunnarmiller
force-pushed
the
demo_v2
branch
from
90275a2 to
1211cc3
Compare
6 participants
Working version of the Demo intended for use at Qubits. Know issues that still need attention are noted below. The first two are formatting issues and should be quick and easy for anyone with the relevant experience with html and CSS. The last one is more serious, with several possible approaches.
KNOWN ISSUES:
Graph Layout:
--The most recent set of changes to enable switching graph views also broke the graph formatting--probably because they created new html.Div wrappers that aren't targeted by the relevant CSS. Someone more conversant with CSS should take a few minutes to fix them. Currently, the graph overlaps with the miner table, and the aspect ratio is not fixed (which it should be). In particular, it elongates enormously if the number of miners is increased too far.
Miner Table:
--There's now a conditional extra column on the miner table, which only appears when one of the "randomized" solver options are selected (which should be often, as they are the defaults). It current lacks a table header section and should have one. It also ends up with widely varying column widths. Ideally, column widths should just be fixed to match the largest entry that could appear in that column.
Pause/Restart Crashes:
There's an elusive issue that occurs sometimes, but not all the time during pausing and restarting the code. It seems to occur much more often with the Bootstrapped solvers, but it does occur occasionally with the QPU solvers, so I think it's worth the time to address even though we want to default the bootstrapped solvers to disabled. I've investigated the issue enough to be reasonably sure that it results from some of the simulation data not being properly saved in the "blockchain-structure" dcc.Store object. Currently, the simulation is supposed to save a full dataset for every block mined so that it can refer back to it when restarting after a pause. There end up being small gaps in the record: some miners missing scores for some blocks, which prevents a clean restart and crashes the demo.
An ideal fix would be to figure out how to save the information more reliably: either troubleshooting the problems with the dcc.Store or finding another way to save it. Failing at that, I could write a patch that could either guess or re-compute a miner's score if it is found to be missing; the down side is that this would result in the restarted chain state being slightly different from the state before the pause, which might be noticeable in some cases. I might be able to work around that. Finally, if both of these prove too difficult, I could simply remove the "Pause" functionality altogether.