Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Fix to include day count in training duration for metrics.json #2718

Merged
merged 6 commits into from
Apr 16, 2019
Merged

Fix to include day count in training duration for metrics.json #2718

merged 6 commits into from
Apr 16, 2019

Conversation

scarecrow1123
Copy link
Contributor

This is a tiny fix to have day count to be included in training_duration in the metrics json. #2717

Sync with allennlp master
Pull from AllenNLP Master
Pull from allennlp master
This fixes Issue [@2717](#2717) to include day count in `training_duration` key in metrics.
Copy link
Contributor

@joelgrus joelgrus left a comment

Choose a reason for hiding this comment

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

technically this isn't quite right either, since it does the wrong thing if your training takes more than 31 days (which is unlikely, but still, might as well fix it for good).

I suspect that strftime isn't the right general solution for this (or at least you'll have to extract the number of days manually)

`time.strftime` does not account for number of days more than
31. Changing it to `datetime.timedelta` and using its `str`
representation for printing epoch duration as well as training
duration.
@scarecrow1123
Copy link
Contributor Author

technically this isn't quite right either, since it does the wrong thing if your training takes more than 31 days (which is unlikely, but still, might as well fix it for good).

I suspect that strftime isn't the right general solution for this (or at least you'll have to extract the number of days manually)

Thanks for the nudge. Yes, strftime may not be a good fit and I've fixed it to use datetime.timedelta. Let me know if that would do.

@joelgrus
Copy link
Contributor

thanks

@joelgrus joelgrus merged commit 7d34ca3 into allenai:master Apr 16, 2019
reiyw pushed a commit to reiyw/allennlp that referenced this pull request Nov 12, 2019
…ai#2718)

* Added default predictor for bimpm model

* Fix allenai#2717: Add day count in training duration

This fixes Issue [@2717](allenai#2717) to include day count in `training_duration` key in metrics.

* Modify elapsed time format to use `timedelta`

`time.strftime` does not account for number of days more than
31. Changing it to `datetime.timedelta` and using its `str`
representation for printing epoch duration as well as training
duration.
TalSchuster pushed a commit to TalSchuster/allennlp-MultiLang that referenced this pull request Feb 20, 2020
…ai#2718)

* Added default predictor for bimpm model

* Fix allenai#2717: Add day count in training duration

This fixes Issue [@2717](allenai#2717) to include day count in `training_duration` key in metrics.

* Modify elapsed time format to use `timedelta`

`time.strftime` does not account for number of days more than
31. Changing it to `datetime.timedelta` and using its `str`
representation for printing epoch duration as well as training
duration.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants