New Dataset and OCR variational pipeline #18
Conversation
…e script/pipeline
…n by default, also a function to change the dropout setting at runtime
…e uncertainty. TODO: calibrate these confidences
…cannot stack classification outputs
…r classification, also added some tests to output the question to a text file
…r classification, also added some tests to output the question to a text file
…into 8-taxi500-dataset
J-Dymond
requested review from
eddableheath and
lannelin
and removed request for
eddableheath
.gitignore
Outdated
|
|
||
|
|
||
| # project related | ||
| data/Taxi1500* |
There was a problem hiding this comment.
presumably not needed now?
There was a problem hiding this comment.
No you're right, we can remove this
pyproject.toml
Outdated
| @@ -34,7 +34,11 @@ dependencies = [ | |||
| "numpy", | |||
| "sentencepiece", | |||
| "librosa", | |||
| "soundfile" | |||
| "soundfile", | |||
There was a problem hiding this comment.
is this necessary? we're not working with audio
There was a problem hiding this comment.
This was left over from the transcription stuff, but yeah we can get rid of this, though it will probably be in all of our virtual environments now, so might it get added again?
requirements.txt
Outdated
There was a problem hiding this comment.
I think this can probably be removed and we can rely on the pyproject? not sure what best practice is
There was a problem hiding this comment.
I'm also not sure, happy to go with what you both think is most appropriate.
scripts/variational_RTC_example.py
Outdated
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| RTC_pars = { |
There was a problem hiding this comment.
snake_case makes it clear when something is a variable vs a class, I'd change this to rtc_params
| # change huggingface cache to be in project dir rather than user home | ||
| export HF_HOME="/bask/projects/v/vjgo8416-spice/hf_cache" | ||
|
|
||
| # TODO: script uses relative path to project home so must be run from home, fix |
There was a problem hiding this comment.
outstanding TODO - I think a simple fix in the data loading, will comment separately.
| def __init__(self, model_pars, data_pars) -> None: | ||
|
|
||
| self.pars = model_pars | ||
| self.OCR = pipeline( |
|
|
||
| return translation | ||
|
|
||
| return self.translator(text)[0]["translation_text"] |
There was a problem hiding this comment.
is this [0] relying on the fact that text is a str and never a list of strings? if so, maybe add a guard to check it is a str
There was a problem hiding this comment.
Yes, I think this assumes a batch size of 1
| recognition = self.OCR(x) | ||
| self.results["OCR"] = recognition["text"] | ||
| self.results["translation"] = self.translate(recognition["text"]) | ||
| self.results["classification"] = self.classify(self.results["translation"]) |
There was a problem hiding this comment.
why store results rather than simply return?
There was a problem hiding this comment.
Since this is the clean pipeline I'm not sure, either way this is deprecated now so perhaps it is best to remove?
There was a problem hiding this comment.
what's the purpose of this file? will we ever use it and not the variational one? should the variational one extend this rather than be completely separate?
There was a problem hiding this comment.
As above, I can remove this now, the variational one has the same functionality when used with clean_inference().
There was a problem hiding this comment.
I think we'll need to link to license under CC-BY-4.0, maybe add a README to this folder?
| set_dropout, | ||
| ) | ||
|
|
||
| # From huggingface page with model: |
There was a problem hiding this comment.
Yeah, this was left over from old versions of this file. Removed.
| new_values[run] = self.var_output[step][run][metric] | ||
| new_var_dict[step][metric] = new_values | ||
| # overwrite the existing output dictionary | ||
| self.var_output = new_var_dict |
There was a problem hiding this comment.
No particular reason, we could maybe refactor this in a separate issue? Or I can change this before we merge?
There was a problem hiding this comment.
I don't think it's a massive change so let's just make it now.
There was a problem hiding this comment.
Functionality looks good.
I resolved the merge conflict for .gitignore as it was stopping the CI checks from running. Those are failing at the moment so could you take a look at why @J-Dymond ? I suspect you haven't got pre-commit installed locally when you're committing up. If you haven't, try:
pip install -e ".[dev]" # from project dir, installs with dev deps
pre-commit install
pre-commit run --all-files
| @@ -259,26 +256,26 @@ def stack_translator_sentence_metrics( | |||
| ] | |||
| return stacked | |||
|
|
|||
| def stack_variational_outputs(self): | |||
| def stack_variational_outputs(self, var_output): | |||
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
This branch includes:
src/arc_spice/data/multieurlex_utils.pysrc/arc_spice/variational_pipelines/RTC_variational_pipeline.pyclean_inferenceandvariational_inferencefunctionality. As well as calculating some confidence metrics on the outputs.src/arc_spice/variational_pipelines/dropout_utils.pysrc/arc_spice/eval/classification_error.pyandsrc/arc_spice/eval/translation_error.pyscripts/variational_RTC_example.py