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

ENH: Add ability to run eval with and without post-processing transforms #621

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

NickleDave
Copy link
Collaborator

This does two things:

  • convert all the transforms for labeled timebins to actual transforms with functional forms and corresponding classes, as in ENH: Convert post-processing to Transform classes that can be specified in config  #537. For details see commit and discussion on that issue.
    • Essentially we factor a postprocess transform out of the function that was in vak.labeled_timebins that is applied to labeled timebins, and then returns labeled timebins (instead of segments or labels only).
    • This allows us to compute metrics like accuracy on the transformed labeled timebins, and also use it to get labels with a to_labels transform, or labels + onset and offset times with to_segments.
  • add the ability to run eval with the same post-processing transforms applied that we use for prediction, as in ENH: eval should have option compute metrics with and without clean-ups #472. We achieve this by adding a post_tfm attribute to engine.Model which defaults to None, and a corresponding parameter post_tfm_kwargs to core.eval and core.learncurve that if specified is used to make an instance of PostProcess that is then passed into the Model when instantiating. If passed in, it is used during eval on the output of the model, e.g. the predicted labeled timebins, as described above.

Before merging we need to do the following (besides making sure CI passes):

  • test that the new version replicates what we got with the TweetyNet paper when we did this "manually" with a script
  • It doesn't have to be perfect but it shouldn't be wildly off base

We will then make a release of vak, and finally we'll need to release a new TweetyNet model that has the post_tfm parameter added to its from_config method. That new release will need to have the new version of vak as its minimum required version.

@NickleDave NickleDave force-pushed the run-eval-with-or-without-cleanup branch 2 times, most recently from 47260fa to 79508b7 Compare February 7, 2023 16:42
@NickleDave NickleDave marked this pull request as ready for review February 8, 2023 14:23
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Base: 93.22% // Head: 93.34% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (adb248e) compared to base (1ae478d).
Patch coverage: 98.25% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #621      +/-   ##
==========================================
+ Coverage   93.22%   93.34%   +0.11%     
==========================================
  Files          66       68       +2     
  Lines        2494     2613     +119     
==========================================
+ Hits         2325     2439     +114     
- Misses        169      174       +5     
Impacted Files Coverage Δ
tests/fixtures/annot.py 93.68% <90.74%> (-5.14%) ⬇️
tests/fixtures/config.py 94.16% <100.00%> (+0.09%) ⬆️
tests/test_core/test_eval.py 100.00% <100.00%> (ø)
tests/test_labeled_timebins.py 100.00% <100.00%> (ø)
tests/test_labels.py 100.00% <100.00%> (ø)
...ransforms/test_labeled_timebins/test_functional.py 100.00% <100.00%> (ø)
...ransforms/test_labeled_timebins/test_transforms.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

- Factor all functions out of labeled_timebins except has_unlabeled
- Only test has_unlabeled in test_labeled_timebins,
  parametrize the unit test
- Add 'multi_char_labels_to_single_char' to vak.labels
  - and add type annotations and clean up docstrings
- Clean up / add tests to tests/test_labels.py
- Add src/vak/transforms/labeled_timebins/functional.py
- Add src/vak/transforms/labeled_timebins/transforms.py
- Fix imports in vak/transforms/ __init__.py files
  - Import labeled_timebins transforms in transforms/__init__.py
  - Fix other imports
  - Import classes from transforms in
    src/vak/transforms/labeled_timebins/__init__.py
- TST: Move paths outside fixtures in fixtures/annot.py
  to be able to import directly in test modules
  to parametrize there.
- TST: Move path to constant outside fixture in fixtures/config.py
- Add tests/test_transforms/__init__.py
  so we can do relative import from tests/fixtures
- Add tests/test_transforms/test_labeled_timebins/test_functional.py
- Add tests/test_transforms/test_labeled_timebins/test_transforms.py
- Use `vak.transforms.labeled_timebins.from_segments`
  in vocal_dataset.py
- Use vak.transforms.labeled_timebins in engine/model.py
- Use transforms.labeled_timebins.from_segments
  in datasets/window_dataset.py
- Use transforms.labeled_timebins.to_segments in core/predict.py
- Use labeled_timebins transforms 'postprocess' and 'to_segments'
  in core/predict.py
@NickleDave NickleDave force-pushed the run-eval-with-or-without-cleanup branch from 79508b7 to 738414c Compare February 8, 2023 23:55
- Add post_tfm_kwargs to config/eval.py
- Add post_tfm_kwargs attribute to LearncurveConfig
- Add 'post_tfm_kwargs' option to config/valid.toml
- Add post_tfm_kwargs to LEARNCURVE section of vak/config/valid.toml
- Add use of post_tfm eval in engine.Model
    - have _eval compute metrics with *and* without post_tfm,
      if post_tfm is specified
    - metrics computed *with* post_tfm have '_tfm' appended to
      metric name in returned `metric_vals`
- Add post_tfm_kwargs to core.eval and use with model
  - Add logic in core/eval.py to use post_tfm_kwargs to make post_tfm
  - Use multi_char_labels_to_single_char in core.eval,
    not in transforms, to make sure edit distance is computed
    correctl
- Add post_tfm parameter to vak.models.from_model_config_map
  - Add parameter and put in docstring,
  - Pass argument into Model.from_config
- Add post_tfm_kwargs to TeenyTweetyNet.from_config
- Add post_tfm_kwargs to unit test in test_core/test_eval.py
- Pass post_tfm_kwargs into core.eval in cli/eval.py
- Add parameter post_tfm_kwargs to vak.core.learncurve function,
  pass into calls to core.eval
- Pass post_tfm_kwargs into core.learncurve inside cli.learncurve
@NickleDave NickleDave force-pushed the run-eval-with-or-without-cleanup branch from 738414c to adb248e Compare February 9, 2023 03:42
@NickleDave
Copy link
Collaborator Author

@yardencsGitHub I was able to get a "good enough" replication of the paper result for one bird by running learncurve, and so I am going to consider this good enough to merge.

Here's the original paper result for bl26lb16:
image

And here's what I got from a re-run where we run eval with and without post-processing transforms at every step:
image

I used previous_run_path so differences here are really about what windows the network saw and floating point error. Qualitatively, trends are the same.

@NickleDave NickleDave merged commit f8acbd2 into main Feb 9, 2023
@NickleDave NickleDave deleted the run-eval-with-or-without-cleanup branch February 9, 2023 14:38
NickleDave added a commit that referenced this pull request Feb 9, 2023
NickleDave added a commit that referenced this pull request Feb 9, 2023
NickleDave added a commit that referenced this pull request Feb 11, 2023
Change `lbl_tb2labels` to an instance of
`transforms.labeled_timebins.ToLabels`.
Fix needed after rebasing version 1.0 on #621
NickleDave added a commit that referenced this pull request Feb 24, 2023
Change `lbl_tb2labels` to an instance of
`transforms.labeled_timebins.ToLabels`.
Fix needed after rebasing version 1.0 on #621
NickleDave added a commit that referenced this pull request Feb 24, 2023
Change `lbl_tb2labels` to an instance of
`transforms.labeled_timebins.ToLabels`.
Fix needed after rebasing version 1.0 on #621
NickleDave added a commit that referenced this pull request Mar 3, 2023
Change `lbl_tb2labels` to an instance of
`transforms.labeled_timebins.ToLabels`.
Fix needed after rebasing version 1.0 on #621
NickleDave added a commit that referenced this pull request Mar 3, 2023
NickleDave added a commit that referenced this pull request Mar 3, 2023
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.

None yet

2 participants