-
Notifications
You must be signed in to change notification settings - Fork 0
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving a partial review before I jump into meetings.
Overall looks good, I've added some comments requesting some small changes.
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outstanding TODO - I think a simple fix in the data loading, will comment separately.
|
||
return translation | ||
|
||
return self.translator(text)[0]["translation_text"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this assumes a batch size of 1
src/arc_spice/variational_pipelines/RTC_variational_pipeline.py
Outdated
Show resolved
Hide resolved
src/arc_spice/variational_pipelines/RTC_variational_pipeline.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/arc_spice/variational_pipelines/RTC_variational_pipeline.py
Outdated
Show resolved
Hide resolved
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! A couple of very minor comments to check but then should be good to merge.
This branch includes:
src/arc_spice/data/multieurlex_utils.py
src/arc_spice/variational_pipelines/RTC_variational_pipeline.py
clean_inference
andvariational_inference
functionality. As well as calculating some confidence metrics on the outputs.src/arc_spice/variational_pipelines/dropout_utils.py
src/arc_spice/eval/classification_error.py
andsrc/arc_spice/eval/translation_error.py
scripts/variational_RTC_example.py