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

Bug on DummyCNV. Only affects CoTeDe tests #141

Open
castelao opened this issue Jan 27, 2016 · 8 comments
Open

Bug on DummyCNV. Only affects CoTeDe tests #141

castelao opened this issue Jan 27, 2016 · 8 comments

Comments

@castelao
Copy link
Member

It was missing a round to define the seconds at DummyCNV. Approximately 50% of the cases would have a 1 second off.

This only affects CoTeDe tests, and it is just a conceptual error. It shouldn't effectively change any result.

s-good added a commit that referenced this issue Jan 27, 2016
BUGFIX #141, missing a round to define the seconds.
@s-good
Copy link
Contributor

s-good commented Jan 27, 2016

Hi @castelao, thanks for this and also for your work adding the Wod4CoTeDe class to wodpy. I guess this means that we can eventually remove the DummyCNV class and use the Wod4CoTeDe class instead?

@castelao
Copy link
Member Author

Hi @s-good, yes, that's the plan. But don't worry about this, you two already have plenty on your plate. I'll be pushing updates as I can move on.

@s-good
Copy link
Contributor

s-good commented Jan 28, 2016

Hi @castelao, something just occurred to me - will the rounding of the number of seconds work if seconds >= 59.5? Will this get rounded up to 60, which isn't a valid number of seconds?

@castelao
Copy link
Member Author

Good call @s-good

Let me try again:
castelao/wodpy@4bd8d0e

@s-good
Copy link
Contributor

s-good commented Jan 28, 2016

This solution looks really good. A lot more elegant than manually calculating the hours, minutes and seconds!

@s-good
Copy link
Contributor

s-good commented Jan 28, 2016

Just thought, you will need to replace the hours = 0, minutes = 0 and seconds = 0 lines in the if statement with time = 0 or similar.

@castelao
Copy link
Member Author

Yes, I realized just after I submitted that.
https://github.com/IQuOD/wodpy/pull/5/files

Thanks!

@bkatiemills
Copy link
Member

Good to close here?

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

No branches or pull requests

3 participants