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 granule duration check, to allow shorter than 48 scans in a granule #23

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

adybbroe
Copy link
Contributor

@adybbroe adybbroe commented Dec 17, 2020

Signed-off-by: Adam.Dybbroe a000680@c21856.ad.smhi.se

  • Closes #xxxx
  • Tests added
  • Tests passed: Passes pytest
  • Passes flake8 nwcsafpps_runner
  • Fully documented

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe adybbroe added the bug label Dec 17, 2020
@adybbroe adybbroe self-assigned this Dec 17, 2020
@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #23 (a0a7441) into master (2b1f47e) will increase coverage by 1.91%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   27.80%   29.71%   +1.91%     
==========================================
  Files          11       11              
  Lines        1705     1740      +35     
==========================================
+ Hits          474      517      +43     
+ Misses       1231     1223       -8     
Flag Coverage Δ
unittests 29.71% <100.00%> (+1.91%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
nwcsafpps_runner/pps_posttroll_hook.py 79.62% <100.00%> (+5.41%) ⬆️
nwcsafpps_runner/tests/test_pps_hook.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b1f47e...a0a7441. Read the comment docs.

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe
Copy link
Contributor Author

@TAlonglong and @mraspaud Can we merge this one?

@mraspaud
Copy link
Member

Not yet, tests are failing and it hasn't been reviewed :)

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Could you make sure that 47 and 48 scans are the only possible values? (maybe add a reference to a document)

Comment on lines 261 to 266
posttroll_message = PostTrollMessage(0, metadata)
delta_t = timedelta(seconds=82.8)

with patch.object(PostTrollMessage, 'get_granule_duration', return_value=delta_t) as mock_method:
result = posttroll_message.is_segment()
self.assertTrue(result)
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a check for 48 scans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had!?
I have a test for 82.2 seconds (47 scans) and 84.1 sec (48 scans).

The thing is, as far as I interpret things, that in the SDR files you have the start and end time in terms of time of the first scan, and time of the last scan. But to get to the duration of the entire granule you need to add the time length of one scan (~1.8 sec). Thus, the duration of 48 scans is ~85.9 sec and ~84.1 seconds for 47 scans.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the test for 84.1 seconds in test_is_segment, what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mraspaud Do you see it now?

@adybbroe
Copy link
Contributor Author

Not yet, tests are failing and it hasn't been reviewed :)

The couple of test-suites that fail (ubuntu/wondows latest), doesn't have anything to do with this PR as I see it.

@adybbroe
Copy link
Contributor Author

Thanks for fixing this. Could you make sure that 47 and 48 scans are the only possible values? (maybe add a reference to a document)

How do you mean? Should we omit PPS processing all together if there are 46 scans or less?

@mraspaud
Copy link
Member

Not yet, tests are failing and it hasn't been reviewed :)

The couple of test-suites that fail (ubuntu/wondows latest), doesn't have anything to do with this PR as I see it.

Could you just add a skip statement then for windows platforms if you don't want to fix it here?

@mraspaud
Copy link
Member

Thanks for fixing this. Could you make sure that 47 and 48 scans are the only possible values? (maybe add a reference to a document)

How do you mean? Should we omit PPS processing all together if there are 46 scans or less?

I'm just unsure what the format specification says about viirs granules. Is is just 47 or 48 scans, or can it be less or more?

…culation

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@adybbroe
Copy link
Contributor Author

Thanks for fixing this. Could you make sure that 47 and 48 scans are the only possible values? (maybe add a reference to a document)

How do you mean? Should we omit PPS processing all together if there are 46 scans or less?

I'm just unsure what the format specification says about viirs granules. Is is just 47 or 48 scans, or can it be less or more?

Yes, me too. From our own processing it looks like it is either 47 or 48 scans, but the actual length varies a bit more than just between two values. So, seems like 48 scans is not always exactly the same size in terms of start and end times in the files! I get betwee 1min24.1sec to 1min24.5 sec (that is actually just end-start times - so missing 1.779 sec) for the 48 scan files.

I have linked the SDR spec already, but don't find the allowed/occuring number of granules.

@adybbroe
Copy link
Contributor Author

I have adjusted the granule length calculation adding length of one granule.

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
@mraspaud
Copy link
Member

About skipping, I meant just skipping these specific tests, with https://docs.pytest.org/en/latest/skipping.html

@adybbroe
Copy link
Contributor Author

About skipping, I meant just skipping these specific tests, with https://docs.pytest.org/en/latest/skipping.html

Okay, thanks, but now I don't see what exactly was failing anymore, but it was something about gdal, and it was windows and ubuntu experimental, so I rather want to fix it in a separate PR then. The github ci was added just recently.
Ok?

@adybbroe
Copy link
Contributor Author

@mraspaud Where are we with this?

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud merged commit c734de3 into pytroll:master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants