pytroll / pytroll-pps-runner

Pytroll runner for PPS
GNU General Public License v3.0
1 stars 8 forks source link

Fix granule duration check, to allow shorter than 48 scans in a granule #23

Closed adybbroe closed 3 years ago

adybbroe commented 3 years ago

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

codecov[bot] commented 3 years ago

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%) :arrow_up:

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%) :arrow_up:
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.

adybbroe commented 3 years ago

@TAlonglong and @mraspaud Can we merge this one?

mraspaud commented 3 years ago

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

adybbroe commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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?

adybbroe commented 3 years ago

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 commented 3 years ago

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

mraspaud commented 3 years ago

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

adybbroe commented 3 years ago

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 commented 3 years ago

@mraspaud Where are we with this?