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

sctk_binary_path variable in Hub5ScoreJob #292

Merged
merged 6 commits into from
Jul 25, 2022

Conversation

DanEnergetics
Copy link
Contributor

The use of binary path variables in the sisyphus.global_settings might become deprecated in the future (as discussed in #254). This is a slight change of the Hub5ScoreJob similar to the ScliteJob in this direction. Moreover, some type annotations have been added to the init signature.

recognition/scoring.py Outdated Show resolved Hide resolved
recognition/scoring.py Show resolved Hide resolved
recognition/scoring.py Outdated Show resolved Hide resolved
DanEnergetics and others added 3 commits July 22, 2022 15:19
Co-authored-by: michelwi <[email protected]>
Co-authored-by: michelwi <[email protected]>
@DanEnergetics
Copy link
Contributor Author

DanEnergetics commented Jul 22, 2022

I added your suggested changes. I thought about removing str in the annotations, as well but wasn't sure, if in that case, we would also like to remove appearances of tk.uncached_path in the Hub5ScoreJob as suggested in #32 the way it's done in ScliteJob already. What's your opinion?

@michelwi
Copy link
Contributor

remove appearances of tk.uncached_path

sure, lets do it.


ref = self.ref
try:
ref = shutil.copy(tk.uncached_path(ref), ".")
ref = shutil.copy(ref.get_path(), ".")
except shutil.SameFileError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering: why are we copying ref and hyp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can this be the same file?? "." should be the job work dir, correct? ergo this should not be the same file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we copying ref and hyp?

dunno.

how can this be the same file??

maybe during automatic LM scale optimization?

@christophmluscher
Copy link
Contributor

moving the copy of ref and hyp to a new issue #293

@christophmluscher christophmluscher self-requested a review July 25, 2022 10:58
@michelwi michelwi merged commit cb0e2a4 into rwth-i6:main Jul 25, 2022
@DanEnergetics DanEnergetics deleted the mann-add-sctk-path-variable branch July 25, 2022 11:54
Atticus1806 pushed a commit that referenced this pull request Jul 26, 2022
* Add sctk_binary_path variable to Hub5ScoreJob and add type annotations.

* Reformat.

* Update type annotations

Co-authored-by: michelwi <[email protected]>

* Update docstring

Co-authored-by: michelwi <[email protected]>

* Update imports.

Co-authored-by: michelwi <[email protected]>

* Remove tk.uncached_path.

Co-authored-by: Daniel Mann <[email protected]>
Co-authored-by: michelwi <[email protected]>
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.

3 participants