Closed johann-petrak closed 3 years ago
Timoeller
commented
3 years ago Hey @johann-petrak thanks for the contribution. Unfortunately we are currently very busy with other work and it might take a while reviewing this PR, especially since it touches points in the inferencer and adaptive_model which might impact other tasks.
I see that there is still your diary.md in there. Also the format of the example is rather different to what we usually have. Users might be confused...?
johann-petrak
commented
3 years ago The xval example has been there for a long time, this is just a minor change to make it do the right thing.
What you see in this PR is the PR 408 which has already been merged (this is the part that affects the adaptive model, but that PR has already been accepted). I am not quite sure why this happens, TBH, as I have updated the fork before creating the branch for this PR.
However, please see issue #819: my suggestion would be to close this PR, also close PR https://github.com/deepset-ai/FARM/pull/818 and instead provide one PR, from a new fork to avoid duplication of history, that would implement all three things which belong together and would make sense to test together:
Would that be OK?
Is there a way to discuss larger contributions with the FARM development team somehow?
Timoeller
commented
3 years ago Would that be OK?
Yes that sounds like a good idea. Please go ahead.
Is there a way to discuss larger contributions with the FARM development team somehow?
Totally, I wrote you an email.
johann-petrak
commented
3 years ago Closing, I will provide a new PR for this (without the annoying history, see https://github.com/deepset-ai/FARM/issues/819#issuecomment-874194965)
Sorry for the confusion.
Should fix #812 and also fix that the example previously did not really work as logging the values was broken.