Skip to content

Draft visualizer #448

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

Merged
merged 53 commits into from
Apr 24, 2025
Merged

Draft visualizer #448

merged 53 commits into from
Apr 24, 2025

Conversation

mzuenni
Copy link
Collaborator

@mzuenni mzuenni commented Apr 5, 2025

Todo input visualizer:

  • bt upgrade needs to read generators.yaml, and if it has a visualizer: key, move the target to input_visualizer/ and remove it from generators.yaml. It should then warn that the visualizer itself needs to be rewritten to no longer read from testcase.in, but from stdin.
  • rename Program.visualizer
  • move to visualize.py

Todo output visualizers:

  • implement this
  • Allow enabling output visualizers for bt run (and bt test?) with a command-line argument
  • The resulting judgeimage.<ext> of the output visualizer can be copied back to data/ when bt generate runs it on the canonical jury solution, but only when there is no input visualizer, because there can be at most one image per test case.
  • Related to the above: should we do something when any input validator or generator happens to write image files? => they get overwritten if any visualizer is available
  • Related to the above: should we do something when the output validator or generator happens to write image files? => they get overwritten if any visualizer is available

@mzuenni
Copy link
Collaborator Author

mzuenni commented Apr 6, 2025

Some Notes:
my implementation uses a different InputVisualizer invocation. The visualizer is called with the input and answer files instead of only reading the input from stdin. On one hand this is closer to the old behaviour BAPCtools always used on the other hand the answer file might be necessary for the visualization especially if its not a valid output. (The name InputVisualizer might be confusing...). It also mkes the invocation of input/output visualizers more similar

@mzuenni mzuenni requested a review from mpsijm April 7, 2025 12:34
@thorehusfeldt
Copy link
Collaborator

Thank you for doing this.

We currently have

	visualizer?:  #command & =~"^/" & !~"\\{count" | null

Should generators.yaml actually support something like this instead?:

visualizer?: ["in" | "out"]: #command | null

In particular the requirement that visualizer invocations start with / is now void, because the directory structures is fixed. So the path should instead be relative to input_visualizer and output_visualizer.

But it’s not clear to me that this shall be specified in generators at all any more. When output_visualizer exists, it will be run, and with a narrowly specified invocation. The same may be true of input_visualizer. So what’s left to specifify? (Honest question.) Sometimes may want to switch visualization off, so there’s case to be made for

visualizer?: ["in" | "out"]: *true | false

I can also see myself passing arguments to a visualizer, so maybe we want

visualizer?: ["in" | "out"]: string | false

instead?

@mpsijm
Copy link
Collaborator

mpsijm commented Apr 9, 2025

But it’s not clear to me that this shall be specified in generators at all any more. When output_visualizer exists, it will be run, and with a narrowly specified invocation. The same may be true of input_visualizer. So what’s left to specify?

Agreed, the current visualizer key in generators.yaml is obsolete now, because the mere existence of the visualizer signals that it should be run. If you want to switch off visualization, we may want to add command-line options like --no{,-input,-output}-visualizer. Or do you mean that you may want to disable the visualizer for certain test groups? And regarding passing arguments to the visualizer, shouldn't those live in test_group.yaml (currently still called testdata.yaml in BAPCtools, rename pending), just like the validator arguments? Perhaps a setting for enabling/disabling the visualizer for a test group should also live in test_group.yaml.

(still didn't have time to look at the code in detail or try it out... just replying to Thore's suggestions, thanks for looking at this! ❤️)

@mzuenni
Copy link
Collaborator Author

mzuenni commented Apr 9, 2025

Should generators.yaml actually support something like this instead?:
[...]
So what’s left to specifify?

I don't think we need to specify anything in generators.yaml if you provide an input_visualizer you already show that you want the visualization. And if you don't want to run it temporarily, there is bt generate --no-visualizer.

If you don't want to run it for some testgroup/test cases you can implemtent this via <input, output>_visualizer_args keys (not that in my implementation visualizers are not called with validator args). We could in theory also add to the spec that _visualizer_args is str | list[str] | False where False indicates that it should not be run... But i don't know if we need that.

@thorehusfeldt
Copy link
Collaborator

thorehusfeldt commented Apr 9, 2025

Even more basic question: what does data/sample/01.pdf mean? (Is it the result of running the input visualizer on 01.in?) That would make sense to me, but then the images in levellinglocks would violate this convention. (Because they show instances plus a solution. ) Do we really need data/sample/01.in.pdf now?

@mpsijm
Copy link
Collaborator

mpsijm commented Apr 9, 2025

If we'd strictly follow the spec, the resulting images of the input/output visualizer would not automatically end up in data/ at all; this is a choice that we make in BAPCtools. Also, according to the spec, the image in data/ is an "Illustration of the test case" (whether that's only input, or both input and answer, is the choice of the problem setter). I already thought about this in #438 (comment), and in summary, I think we should do something like this (EDIT and from @mzuenni's comment below, it looks like this is the current implementation):

  • If the input visualizer produces an image, we place that in data/.
  • Else, if the output visualizer produces an image with the canonical jury submission (i.e. the one from which we generate .ans files), we place that in data/.
  • If both produce an image, the result of the output visualizer only stays in feedback_dir (this probably already happens for the non-canonical jury submissions anyway).

So I don't think we need to disambiguate 1.{in,ans}.png, unless you think we need multiple images per test case in some cases (currently, the spec only allows one image file per test case, which I think is fine).

@mzuenni
Copy link
Collaborator Author

mzuenni commented Apr 9, 2025

Even more basic question: what does data/sample/01.pdf mean? (Is it the result of running the input visualizer on 01.in?)

Short answer (in terms of BAPCtools): yes.
More detailed answer: Its the visualization of the testcase (what exactly that means is up to you/the problem). In terms of bt generate its the output of the input_visualizer but if no input_visualizer exists we try to use the output_visualizer as a fallback to generate a visualization of the testcase.

Do we really need data/sample/01.in.pdf now?

I don't see the need for this

@mzuenni
Copy link
Collaborator Author

mzuenni commented Apr 12, 2025

@mpsijm will you have time to take a look?

@mzuenni mzuenni marked this pull request as ready for review April 12, 2025 21:26
Copy link
Collaborator

@mpsijm mpsijm 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 so much for picking this up! ❤️ I see a lot has changed, and the majority is looking very good 😄 Some nit-pick comments or questions below 🙂

I didn't test super extensively, but I'm very happy that @thorehusfeldt also took the time to write some visualizers! ❤️

Copy link
Collaborator

Choose a reason for hiding this comment

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

In https://icpc.io/problem-package-format/spec/2023-07-draft.html#reporting-additional-feedback, it is mentioned that the output validator can also write image files to the feedback directory. Currently, bt generate only runs the answer validator, but I guess that we may also want to run the output validator on the .ans file, if an output validator exists? I think this could be added to TestcaseRule.validate_ans_and_out?

Copy link
Collaborator Author

@mzuenni mzuenni Apr 15, 2025

Choose a reason for hiding this comment

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

this already happens right?
if there is a .out we run the output validator and if there ans_is_output is true the output validator is used as answer validator and therefore also run.

Copy link
Collaborator

@mpsijm mpsijm Apr 15, 2025

Choose a reason for hiding this comment

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

Sure, but I'm talking about the case where a .out file does not exist, which is the case for all secret test cases.

Take for example Jib Job. Say that, as implementor of that problem, I want to visualize how the output of submissions looks like, and use this as test case image as well. This problem could have an output_visualizer like this:

output_visualizer
#!/usr/bin/env python3
import sys

f = open(sys.argv[1]).readlines()
n = int(f[0])
team_jibs = list(map(int, sys.stdin.read().split()))
assert len(team_jibs) == n

xl, xh, yl, yh = 20000, -10000, 20000, -10000

cranes = []
for line, r in zip(f[1:], team_jibs):
    x, y, h = map(int, line.split())
    cranes.append((x, y, r))

    xl = min(xl, x - r)
    yl = min(yl, y - r)
    xh = max(xh, x + r)
    yh = max(yh, y + r)

w = xh - xl
h = yh - yl
if w > h:
    h *= 1024 / w
    w = 1024
else:
    w *= 1024 / h
    h = 1024

with open(sys.argv[3] + "/judgeimage.svg", "w") as of:
    of.write(
        f'<svg xmlns="http://www.w3.org/2000/svg" viewBox="{xl} {yl} {xh - xl} {yh - yl}" width="{w}" height="{h}" style="fill:rgba(0,0,255,0.5)">'
    )
    for x, y, r in cranes:
        of.write(f'<circle cx="{x}" cy="{y}" r="{r}" />')
    of.write("</svg>")

I don't need an input_visualizer (or "test case visualizer"), because it will do exactly the same as the output visualizer, with the difference that it would read from a fixed .ans file, rather than read a submission's output from stdin.

Now, I could also decide to write similar code like this in the output validator instead, rather than having a separate output visualizer. But, currently, the output validator is not run during bt generate for this problem.

(to test this, I currently just appended this to the main function of the output validator of Jib Job, because I didn't feel like actually writing the same visualizer in C++ 😛)

    ofstream vis(string(argv[3]) + "/judgeimage.svg");
    vis << n << endl;
    vis.close();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the case where .out does not exist (and ans_is_out) the output Validator should already be run (as well as any other answer validator)?

we just don't copy the image... at least thats how i understand the code right now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mhm this is kind of ugly for hashing... we now have multiple programs that could create the same file... and we don't know which is actually responsible ^^' This would mean that if either validator or visualizer change both need to be rerun?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think even the current implementation of running the output_visualizer is broken in regards to caching...
The output visualizer can read stuff written by the output_validator but the curren't implementation can't guarantee that these files still exist...

Copy link
Collaborator Author

@mzuenni mzuenni Apr 16, 2025

Choose a reason for hiding this comment

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

I have been thinking about this a lot and I think for interactive or multi-pass problems using the output_visualizer is a bad idea since it visualized the solution which is not part of the testcase.

For ans_is_outout problems or those with a .out file we can use the feedback dir, we just have no guarantee in what order input, output, and answer validators were run...

Copy link
Collaborator

Choose a reason for hiding this comment

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

the output_visualizer [...] visualized the solution which is not part of the testcase

But it makes sense that the output visualizer visualizes a possible output that represents a solution, right?

we just have no guarantee in what order input, output, and answer validators were run...

Aren't we the ones writing the code? 😛 Especially during bt generate: either all validators are run (and I assume in the order "input"-"answer"-"output"), or they are all suppressed with --no-validators. I guess we could make --no-validators imply --no-visualizer because it may be the case that the visualizer reads files from the feedback dir that were left behind by the validators, but to be fair, this will probably not happen very often, so I don't think we need to add this restriction. It may go wrong in some rare cases, but I guess that's fine?

Copy link
Collaborator Author

@mzuenni mzuenni Apr 17, 2025

Choose a reason for hiding this comment

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

Aren't we the ones writing the code? 😛

yes but there are these weird cases... for example if testcases are symlinked because of input:. In that case some of the validators are rerun ^^

But it makes sense that the output visualizer visualizes a possible output that represents a solution, right?

not sure about this ^^' it probably depends on the kind of problem. for guess for example i think its quite strange to visualize a binary search?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm fine with not (fully) supporting visualizers for interactive test cases for now. Same as for the question of an output validator producing image files, we may want to document this somewhere 🙂

@mpsijm mpsijm force-pushed the draft-visualizer branch from 608b5b1 to 9aa7cd4 Compare April 14, 2025 20:33
@mpsijm
Copy link
Collaborator

mpsijm commented Apr 20, 2025

Updated the spec in Kattis/problem-package-format#439 (and hopefully also correctly updated BAPCtools accordingly, I'll check tomorrow if the tests passed) 😄

@mpsijm mpsijm force-pushed the draft-visualizer branch from 217eb03 to 0d2333f Compare April 21, 2025 08:37
@mzuenni mzuenni merged commit baae8d8 into draft Apr 24, 2025
6 checks passed
@mzuenni mzuenni deleted the draft-visualizer branch April 24, 2025 19:34
@mzuenni mzuenni mentioned this pull request Apr 24, 2025
17 tasks
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.

3 participants