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

fix insufficient precision issue in T+dT syntax #1118

Merged
merged 3 commits into from Aug 5, 2021

Conversation

tnakazato
Copy link
Contributor

I noticed that timerange selection by "T+dT" syntax omits the sub-seconds value. More specifically, if we write "HH:MM:SS.ss", ".ss" part is omitted in parsing "T+dT" string. I think this is a bug because "T1~T2" syntax handles sub-seconds value properly. This pull request is possible fix for the bug.

@tammojan
Copy link
Contributor

Well spotted, thanks for the fix! Could you perhaps add a test for this? This could be done by altering tMSTimeGram.run (copying the structure from tMSSelection.run), and making a reference output tMSTimeGram.out. The measurement set mssel_test_small.ms should contain enough time slots to come up with a useful test, in particular it has 23 time slots spaced 1.92 seconds apart.

@tnakazato
Copy link
Contributor Author

Hi, @tammojan. I will try adding the test. By the way, I pushed further improvement using MVEpoch.

@tnakazato
Copy link
Contributor Author

tnakazato commented Jul 29, 2021

Added minimal set of verification tests in tMSTimeGram. The test checks if "T+dT" works properly. As a reference, "T1~T2" test, which selects effectively the same time range, is also added. I confirmed that the test passes on the branch while fails with the code before fix.

@tnakazato
Copy link
Contributor Author

Any update on this? Please let me know if you have more suggestion. Thanks!

@tammojan
Copy link
Contributor

tammojan commented Aug 5, 2021

Thank you, this change looks good to me. Also good to have TimeGram tested now!

@tammojan tammojan merged commit 2a66c0f into casacore:master Aug 5, 2021
@tnakazato
Copy link
Contributor Author

Thanks!

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.

None yet

2 participants