Skip to content

MTG LI L2: Hard coded the satellite position with the nominal vallues #3137

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jequierz
Copy link

In the MTG LI L2 reader li_l2_nc, I have hard coded the nominal satellite position as given in the L1 FCI dataset. It allows to use functions such as get_satpos from satpy.utils that is necessary to apply a parallax correction to the data.

@jequierz jequierz requested review from djhoese and mraspaud as code owners May 19, 2025 13:49
Copy link

codecov bot commented May 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.22%. Comparing base (d3842c8) to head (9449677).
Report is 52 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3137   +/-   ##
=======================================
  Coverage   96.22%   96.22%           
=======================================
  Files         398      398           
  Lines       57383    57388    +5     
=======================================
+ Hits        55218    55223    +5     
  Misses       2165     2165           
Flag Coverage Δ
behaviourtests 3.87% <0.00%> (-0.01%) ⬇️
unittests 96.31% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coveralls
Copy link

coveralls commented May 19, 2025

Pull Request Test Coverage Report for Build 15140857231

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 96.33%

Totals Coverage Status
Change from base Build 15083783462: 0.001%
Covered Lines: 55483
Relevant Lines: 57597

💛 - Coveralls

Copy link
Member

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

Still needs a unit test, and I suggested some other changes.

Comment on lines 738 to 740
logger.info("The satellite position is the nominal position and thus does not" \
"correspond to the actual position of the satellite. The exact satellite position can be found in " \
"FCI level 1 data. ")
Copy link
Member

Choose a reason for hiding this comment

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

I find the formulation here unclear. We should remember that logging messages may appear out of context for most users (they don't call add_orbital_parameters directly, but it gets called as part of larger processing). Maybe something like:

Assuming nominal satellite position for LI L2. The exact position is available from FCI L1.

And I would report this in debug mode, not info mode, as most users will not care about the difference. One use case is parallax correction — for this use case, the difference is too small to be relevant.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I will change this :)

Comment on lines +741 to +744
data_array.attrs["orbital_parameters"] = {"satellite_nominal_longitude": 0.0,
"satellite_nominal_latitude": 0.0,
"satellite_nominal_altitude": 35786400.0
}
Copy link
Member

Choose a reason for hiding this comment

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

If at least the satellite number is known from the metadata (is it?) this hardcoded position could be sorted by satellite. MTG-I2 / Meteosat-14 will have a different position.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I didn't think about the fact that other satellite's products would require the use of the LI reader... But it is true that in any case, it would be better to have a more general solution because apparently the same problem holds for any L2 product of the MTG.
However, I am not sure to understand, how does the satellite number can be used to for this application? By writing a dict of satellites containing a dict of nominal position for each satellite?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would have a dict of satellites. Those should rarely change, although if they do, it will cause more of a headache as we would need to cover old and new data.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, when we start moving the MTGs between the nominal, rapid scan , IODC position etc, we'll have a new headache. But maybe by then we can include the information in the file format... So for now a simple dict mapping the platform_name to the satellite_nominal_longitude should be good for a while. Note that the platform_name was added only recently in #2993

@@ -733,6 +733,17 @@ def update_array_attributes(self, data_array, ds_info):

return data_array

def add_orbital_parameters(self, data_array):
"""Hard code the nominal position of the satellite in the orbital parameters attribute."""
logger.info("The satellite position is the nominal position and thus does not" \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
logger.info("The satellite position is the nominal position and thus does not" \
logger.info("The satellite position is the nominal position and thus does not "

without a trailing space (or a leading one on the next line), it's going to output "does notcorrespond to".

def add_orbital_parameters(self, data_array):
"""Hard code the nominal position of the satellite in the orbital parameters attribute."""
logger.info("The satellite position is the nominal position and thus does not" \
"correspond to the actual position of the satellite. The exact satellite position can be found in " \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"correspond to the actual position of the satellite. The exact satellite position can be found in " \
"correspond to the actual position of the satellite. The exact satellite position can be found in "

@@ -766,5 +777,8 @@ def get_dataset(self, dataset_id, ds_info=None):
# Update the attributes in the final array:
data_array = self.update_array_attributes(data_array, ds_info)

#Add the nominal sat position in attrs orbital parameters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#Add the nominal sat position in attrs orbital parameters
# Add the nominal sat position in attrs orbital parameters

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.

4 participants