Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates Caveat’s evaluation functionality to the external acteval project, removing the in-repo caveat.evaluate implementation and updating dependencies, examples, and CI/docs workflows accordingly.
Changes:
- Remove
caveat.evaluatesource + its unit tests, and addactevalas a dependency. - Update runtime imports (e.g., runners) and example notebooks to use
actevalAPIs. - Update CI/docs/release workflows to install the new PyPI dependency set.
Reviewed changes
Copilot reviewed 67 out of 74 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_eval/test_report.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_frequency.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_filter.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_features_utils.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_features_transitions.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_features_times.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_features_structural.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_features_participation.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_distance_scalar.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_distance_hamming.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_distance_emd.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_distance_edit_distance.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_describe_features.py | Remove legacy evaluate tests (moved to acteval). |
| tests/test_eval/test_creativity.py | Remove legacy evaluate tests (moved to acteval). |
| tests/bench_evaluate.py | Add benchmark harness for evaluation pipeline. |
| requirements/pip.txt | Add PyPI-only deps list (includes acteval). |
| pyproject.toml | Include requirements/pip.txt in dynamic dependencies. |
| mkdocs.yml | Fix MkDocs Material emoji generator reference. |
| examples/4_creativity.ipynb | Switch creativity example to acteval imports + refreshed outputs/metadata. |
| examples/3_NTS_population_demo.ipynb | Switch plotting imports to acteval; clear execution count. |
| examples/2_synthetic_population_gen.ipynb | Switch plotting imports to acteval; clear execution count. |
| caveat/runners.py | Import evaluation/reporting from external acteval. |
| caveat/models/seq2seq/lstm.py | Docstring/type tweaks to predict_step. |
| caveat/models/seq2score/lstm.py | Docstring/type tweaks to predict_step. |
| caveat/models/schedule2label/feedforward.py | Update loss function docstring argument names. |
| caveat/models/joint_vaes/jvae_continuous_rerouted.py | Docstring param rename (device). |
| caveat/models/joint_vaes/jvae_continuous.py | Docstring param rename (device). |
| caveat/models/discrete/vae_discrete_xattention.py | Docstring param rename (device). |
| caveat/models/discrete/auto_discrete_lstm.py | Docstring param rename (top_sampler). |
| caveat/models/continuous/vqvae_lstm.py | Docstring param rename (device). |
| caveat/models/continuous/vae_attention.py | Docstring param rename (device). |
| caveat/models/continuous/cvqvae_lstm.py | Docstring param rename + add labels doc. |
| caveat/models/continuous/cvae_lstm_nudger_adversarial.py | Docstring clarify predict args. |
| caveat/models/continuous/cvae_lstm_nudger.py | Docstring clarify predict args. |
| caveat/models/continuous/cvae_lstm_nudge_feed.py | Docstring clarify predict args. |
| caveat/models/continuous/cvae_lstm_double_nudger.py | Docstring clarify predict args. |
| caveat/models/continuous/cvae_lstm.py | Docstring clarify decode/predict args. |
| caveat/models/continuous/auto_attention.py | Docstring + argument naming updates in attention model. |
| caveat/models/cat_base.py | Docstring param rename (device). |
| caveat/models/base.py | Docstring param rename (device). |
| caveat/jrunners.py | Add type annotation + clarify jbatch_command args. |
| caveat/evaluate/ops.py | Remove in-repo evaluation ops (now external). |
| caveat/evaluate/filters.py | Remove in-repo evaluation filters (now external). |
| caveat/evaluate/features/utils.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/features/transitions.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/features/times.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/features/structural.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/features/participation.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/features/frequency.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/features/creativity.py | Remove in-repo evaluation features (now external). |
| caveat/evaluate/evaluate.py | Remove in-repo evaluation driver (now external). |
| caveat/evaluate/distance/wasserstein.py | Remove in-repo evaluation distance impl (now external). |
| caveat/evaluate/distance/scalar.py | Remove in-repo evaluation distance impl (now external). |
| caveat/evaluate/distance/init.py | Remove in-repo evaluation distance exports. |
| caveat/evaluate/describe/transitions.py | Remove in-repo evaluation plotting (now external). |
| caveat/evaluate/describe/times.py | Remove in-repo evaluation plotting (now external). |
| caveat/evaluate/describe/frequency.py | Remove in-repo evaluation plotting (now external). |
| caveat/encoding/discrete.py | Fix spelling + clarify decode docstrings. |
| caveat/data/samplers.py | Add/adjust type hints for sampling utilities. |
| README.md | Add step to install PyPI dependencies from requirements/pip.txt. |
| .github/workflows/release.yml | Inline conda upload + docs deploy steps; install pip deps. |
| .github/workflows/pre-release.yml | Inline conda build steps. |
| .github/workflows/pr-ci.yml | Inline CI steps; install pip deps; upload coverage. |
| .github/workflows/docs.yml | Inline docs build/deploy steps; install pip deps. |
| .github/workflows/daily-scheduled-ci.yml | Inline scheduled CI steps; install pip deps. |
| .github/workflows/commit-ci.yml | Inline commit CI steps; install pip deps. |
Comments suppressed due to low confidence (3)
caveat/models/seq2score/lstm.py:110
- This
predict_stepoverride is not compatible with PyTorch Lightning:Trainer.predict()will callpredict_step(self, batch, batch_idx, ...), so the current signature(batch, device: int, ...)will raise aTypeError. Either remove this override to useExperiment.predict_step, or update the signature to acceptbatch_idx(and rely on Lightning for device placement). Also, the return annotation/docstring sayTensor, but the method returns a tuple.
caveat/models/continuous/auto_attention.py:90 decode()is annotated to returnTuple[Tensor, Tensor], but it returns a singleTensor(log_probs). This mismatch will confuse callers and static type checkers; update the return annotation (and/or implementation) so they agree.
caveat/models/seq2seq/lstm.py:190- This
predict_stepoverride is not compatible with PyTorch Lightning:Trainer.predict()will callpredict_step(self, batch, batch_idx, ...), so the current signature(batch, device: int, ...)will raise aTypeError. Either remove this override to useExperiment.predict_step, or change the signature to acceptbatch_idx(and avoid passing an explicitdevice, since Lightning already moves tensors to the right device). Also, the return annotation/docstring sayTensor, but the method returns a tuple.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+14
to
+16
| from caveat.data.synth import ActivityGen | ||
| from caveat.evaluate.evaluate import process_metrics | ||
|
|
Comment on lines
+1
to
+6
| """Benchmark harness for caveat.evaluate. | ||
|
|
||
| Usage: | ||
| python tests/bench_evaluate.py --population 1000 --activities 10 | ||
| python tests/bench_evaluate.py --population 5000 --activities 15 | ||
| """ |
| def sample_data( | ||
| sequences: DataFrame, attributes: Optional[DataFrame], config: dict | ||
| ): | ||
| ) -> tuple: |
Comment on lines
105
to
108
| def predict_sequences( | ||
| self, current_device: int, **kwargs | ||
| ) -> Tuple[Tensor, Tensor]: | ||
| """Given samples from the latent space, return the corresponding decoder space map. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
evaluate module split into the acteval project, now external.