Stores Timechop image to disk

[nanounanue]

Related to #512.

Doesn't show the plot, but it stores it to disk

[nanounanue]

@thcrock Could you point me to an example of ProjectStore? Maybe when triage stores the profile?

[codecov-io]

Codecov Report

Merging #590 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
- Coverage   82.49%   82.46%   -0.04%     
==========================================
  Files          83       84       +1     
  Lines        4845     4905      +60     
==========================================
+ Hits         3997     4045      +48     
- Misses        848      860      +12

Impacted Files	Coverage Δ
src/triage/cli.py	0% <0%> (ø)	⬆️
src/triage/util/db.py	100% <0%> (ø)	⬆️
...c/versions/0bca1ba9706e_add_matrix_uuid_to_eval.py	0% <0%> (ø)
src/triage/component/results_schema/schema.py	97.72% <0%> (+0.12%)	⬆️
src/triage/component/catwalk/evaluation.py	97.54% <0%> (+0.12%)	⬆️
src/triage/component/catwalk/storage.py	92.88% <0%> (+0.48%)	⬆️

---

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 18726a0...d3bb1d6. Read the comment docs.

[thcrock]

We did cover it in slack, but for anybody else reading here is where the project storage is used to save profiling data. https://github.com/dssg/triage/blob/master/src/triage/experiments/base.py#L661-L673

[nanounanue]

@thcrock How do you suggest to get a reference to the ProjectStore object, It needs the experiment information. Should we create it in the cli.py or inside timechop component (How do we get the reference there?)

[thcrock]

You already have the project path, which is all you need to instantiate a new ProjectStorage object. This is all the Experiment is doing to create it: https://github.com/dssg/triage/blob/master/src/triage/experiments/base.py#L102

For now, for consistency with the experiment I'd probably instantiate the ProjectStorage within the timechop component.

[nanounanue]

Perfect!

[nanounanue]

@thcrock in order to have access to the storage component, I removed the ShowTimeChop class from cli.py and converted to a argument inside Experiment class. I was looking at ProjectStorage and it seems to me that your suggestion is create a new storage class? like the ones ModelStorage and MatrixStorage?

[thcrock]

No, I don't think there necessarily needs to be a new class like ModelStorage, you could use ProjectStorage directly. Here's an example:

from triage.components.catwalk.storage import ProjectStorage

...
project_storage = ProjectStorage(args.project_path)
timechop_store = self.project_storage.get_store(
            ["images"],
            f"timechop_{<whatever_unique_identifier_you_want>}"
        )
with timechop_store.open('wb') as fd:
    visualize_chops(chopper,..., save_target=fd) # at least I think save_target can take a filehandle

Whether or not it goes in the 'triage experiment' command or stats in the 'triage showtimechops' command, I don't have a strong opinion on. My goal with the 'triage showtimechops' command is that you could send it an experiment config but could also just give it a YAML file that only has temporal config, so reusing that is fine. Both interfaces are fine I think, and there's no reason they couldn't co-exist.

[nanounanue]

@thcrock Thanks! I will do the changes that you suggested

[nanounanue]

@thcrock done. I am gonna merge this. Should I use squash and merge?

[jesteria]

Not sure about what this does to the experiment command

[jesteria]

...This was intentional? To change the validate default from True to False?

[jesteria]

It seems the intention is to add to the existing "don't run" option, validate_only, a new one, show_timechop.

I don't know if changing the validate default was intended to help with this; though, I don't think our desire to validate, by default, has changed.

This allows for what I think would be nonsensical invocations, however, e.g.:

--validate --show-timechop

...which won't show any timechop, no?

If I'm right, I'd recommend:

1. Don't change the defaults, (if they still hold for how we want to run the experiment).
2. Move this elif to check for show_timechop first -- it's not a default, so if it's set, they definitely wanted it
3. Add some of these arguments exactly the same, but to a mutually-exclusive group, (rather than directly onto the parser) [docs]. I would think ('--validate', '--validate-only', '--show-timechop'), for example, are such a mutually-exclusive group -- it doesn't make sense for the user to mix these on the command line.