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

export coefficients to json file #58

Merged
merged 38 commits into from
Jun 11, 2020
Merged

Conversation

carloshorn
Copy link
Collaborator

@carloshorn carloshorn commented Apr 23, 2020

In favour of giving more control over the calibration coefficients to the user, this PR will export the coefficients from calibration.py to an external data file.

Closes #62

@carloshorn
Copy link
Collaborator Author

Next step is the user interaction. I would add another section into the pygac config which gives the possibility to add the path to a user defined calibration.json file.
I think it would be good if the user could give a reduced json file which is used to update the default coefficients, or do you think that this makes it harder to track the source?
I would of cause log any changed coefficient.

@sfinkens
Copy link
Member

Being able to just override a small subset of coefficients would be a nice feature! Users focused on traceability can still download an entire set of coefficients via DOI or so.

I'm ok with the config section. Can you please also make this an argument of Reader.__init__? Then it can be used in satpy like so: https://github.com/pytroll/satpy/blob/master/satpy/readers/avhrr_l1b_gaclac.py#L130

@carloshorn
Copy link
Collaborator Author

Good idea @sfinkens.
Regarding the user friendliness, we should also talk about the coefficient names. These names should be more self-explanatory. Suggestions are welcome! I will try to come up with a list to start the iteration.

@mraspaud
Copy link
Member

Regarding the coeffs names, I would also vote for having a documentation page explaining each coeff and the formulas they are applied in.

Comment on lines 48 to 49
key: cls.parse(custom.get(key) or default.get(key))
for key in cls.fields
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
key: cls.parse(custom.get(key) or default.get(key))
for key in cls.fields
**default, **custom

Copy link
Member

Choose a reason for hiding this comment

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

In principle this take default values from default and uses custom coeffs when availble

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only available for python 3.5+, so it won't survive the CI checks for python 2.7... The backward compatible way I found is:

spacecraft_coeffs = default.copy()
spacecraft_coeffs.update(custom)

In my opinion, we can do without python 2 compatibility, but I do not know if it is also your intention.

@carloshorn
Copy link
Collaborator Author

And again a feature missing in python 2.... In ancient times, the ConfigParser.get method did not know fallback values....

@carloshorn
Copy link
Collaborator Author

With the last commits, the user can provide different default coefficients via the config file
in the section "calibration" as option "coeffs_file", e.g.

[calibration]
coeffs_file = /path/to/user/default/coeffs.json

If coeffs_file is not given, it will fallback to the pygac defaults in data/calibration.json. Furthermore, it is possible to overwrite the defaults by passing custom coefficients directly to the calibrate_solar, and calibrate_thermal functions.

This covers the three use cases:

  1. The user does not care about calibration details and wants to use whatever pygac recommends. This is the default behaviour and the user does not need to do anything.
  2. The user has computed his own coefficients and wants to use them instead of the pygac recommendation. In this case, the user can compile these coefficients in the format of data/calibration.json and provide it via the pygac configuration.
  3. The user only wants to change a small subset of coefficients, e.g. for a sensitivity study. In this case, the user can provide the subset of coefficients to change with a dictionary directly to the calibration functions (which is passed to the reader constructor as "calibration" keyword argument).

So far the implementation of this feature... Now, we should decide for a good naming convention of these parameters.
How about using the naming convention from https://github.com/rsgb-neuhaus/viscal_db/blob/master/csv/header.patmos.csv?
This would also avoid list type objects in the json file.

@abhaydd
Copy link
Collaborator

abhaydd commented May 12, 2020

Great job. Thanks a lot @carloshorn for this adaptation.

@sfinkens
Copy link
Member

Nice work @carloshorn ! The viscal_db names look good to me.

@carloshorn
Copy link
Collaborator Author

carloshorn commented May 14, 2020

Having in mind that I wanted to avoid list types in the json to give every item a specific name, I came up with the following naming convention, which makes use of the hierarchical json structure.

"metopb": {
    "channel_1": {
        "dark_count": 39.7,
        "gain_high_s0": 0.166,
        "gain_high_s1": 2.019,
        "gain_high_s2": -0.201,
        "gain_low_s0": 0.055,
        "gain_low_s1": 2.019,
        "gain_low_s2": -0.201,
        "gain_switch": 501.12
    },
    "channel_2": {
        "dark_count": 40.0,
        "gain_high_s0": 0.183,
        "gain_high_s1": 1.476,
        "gain_high_s2": -0.137,
        "gain_low_s0": 0.061,
        "gain_low_s1": 1.476,
        "gain_low_s2": -0.137,
        "gain_switch": 500.82
    },
    "channel_3a": {
        "dark_count": 40.3,
        "gain_high_s0": 0.201,
        "gain_high_s1": 1.748,
        "gain_high_s2": -0.033,
        "gain_low_s0": 0.029,
        "gain_low_s1": 1.748,
        "gain_low_s2": -0.033,
        "gain_switch": 501.32
    },
    "channel_3b": {
        "centroid_wavenumber": 2664.3384,
        "radiance_correction_c0": 0.0,
        "radiance_correction_c1": 1.0,
        "radiance_correction_c2": 0.0,
        "space_radiance": 0.0,
        "to_eff_blackbody_intercept": 0.9970158319134996,
        "to_eff_blackbody_slope": 1.7711318
    },
    "channel_4": {
        "centroid_wavenumber": 933.71521,
        "radiance_correction_c0": 5.44,
        "radiance_correction_c1": 0.89848,
        "radiance_correction_c2": 0.00046964,
        "space_radiance": -4.98,
        "to_eff_blackbody_intercept": 0.9986240957209157,
        "to_eff_blackbody_slope": 0.51860807
    },
    "channel_5": {
        "centroid_wavenumber": 839.72764,
        "radiance_correction_c0": 3.84,
        "radiance_correction_c1": 0.93751,
        "radiance_correction_c2": 0.00025239,
        "space_radiance": -3.4,
        "to_eff_blackbody_intercept": 0.9988311677674785,
        "to_eff_blackbody_slope": 0.40059787
    },
    "date_of_launch": 2012.77,
    "thermometer_1": {
        "c0": 276.6194,
        "c1": 0.050919,
        "c2": 1.471e-06,
        "c3": 0.0,
        "c4": 0.0
    },
    "thermometer_2": {
        "c0": 276.6511,
        "c1": 0.050892,
        "c2": 1.489e-06,
        "c3": 0.0,
        "c4": 0.0
    },
    "thermometer_3": {
        "c0": 276.6597,
        "c1": 0.050845,
        "c2": 1.521e-06,
        "c3": 0.0,
        "c4": 0.0
    },
    "thermometer_4": {
        "c0": 276.3685,
        "c1": 0.050992,
        "c2": 1.482e-06,
        "c3": 0.0,
        "c4": 0.0
    }
}

Even if some coefficients have the same prefix, I think that two levels provide a good readability. The only thing that I would like to change is the launch date format. A fraction of year is not very intuitive. I would prefer a classical timestamp like YYYY-mm-dd. What do you think?

Edit: Maybe someone can come up with a better name for to_eff_blackbody, which should point out that this coefficients should be used for the transformation from thermometer temperature to effective black body temperature. as defined in (7.1.2.4-3) KLM Users Guide.

@mraspaud
Copy link
Member

Looks good! for the thermometer coeffs, why not skip c3 and c4 as they don't seem to be relevant ?

@carloshorn
Copy link
Collaborator Author

I would prefer to keep c3 and c4, even if always set to zero, because it is equation (7.1.2.4-1) in the KLM guide which defines them. They are used in the code in the following lines

pygac/pygac/calibration.py

Lines 495 to 499 in 2d7fbaf

tprt = (cal.d[iprt, 0] + prt *
(cal.d[iprt, 1] + prt *
(cal.d[iprt, 2] + prt *
(cal.d[iprt, 3] + prt *
(cal.d[iprt, 4])))))

If we default them to zero in the code, I would not see the point of having a default value file.

@mraspaud
Copy link
Member

Good point, I agree, thank you for explaining this :)

@carloshorn
Copy link
Collaborator Author

I will start the implementation of the new structure tomorrow. I would also like to put more comment lines in the calibration functions and maybe rename some inner variables, but it should neither chnage the results, nor should it change the interface (function argument names). Are you okay with adding it into this PR?

@mraspaud
Copy link
Member

Go ahead.

@carloshorn
Copy link
Collaborator Author

Even comment lines can break python 2.7....

@carloshorn
Copy link
Collaborator Author

Cleanup and finalisation plan: if we agree on the coefficients, I would remove the old2new and new2old functions and do the correct mapping inside the Calibrator class. I used these functions to create the json file from the original coeffs dictionary. I also used them to make sure that the results never changed by reproducing the original coefficients with the reverse call.
My question here: Should I keep all the rounding calls used in new2old to exactly reproduce the original results? Note, that the results might have slightly changed for NOAA-7 in any case due to the corrected typo.
I introduced a deprecation warning in the solar calibration when using the correction factor argument corr in order to keep the units of the output clear. I could make sure that the transformation from scaled radiance to isotropic radiance is done outside in the reader class. The other possibility would be a change of interface by adding the sun-earth distance correction factor and solar-zenith-angle to the argument list and directly output isotropic radiance. Any preference?

@helgaweb
Copy link

helgaweb commented Jun 3, 2020

@carloshorn Looks and sounds very good! Thanks!
Any specific reason why you did not include the KLM User's guide in the method reference for the thermal calibration in the json file?

@carloshorn
Copy link
Collaborator Author

Any specific reason why you did not include the KLM User's guide in the method reference for the thermal calibration in the json file?

No... I simply forgot it...

@helgaweb
Copy link

helgaweb commented Jun 3, 2020

Nice! I would prefer the optional correction for Sun-earth-distance in the reader class. Makes the code cleaner.

@sfinkens
Copy link
Member

sfinkens commented Jun 5, 2020

I think it's good if the coefs are identical, so I'd say keep the rounding. I don't have an opinion about the scaled/isotropic radiance thing :)

@mraspaud
Copy link
Member

mraspaud commented Jun 6, 2020

I agree with keeping the rounding. Is the scaled radiance being output at the moment to hdf files?

@helgaweb
Copy link

helgaweb commented Jun 6, 2020

Yes.

def calibrate_solar(counts, chan, year, jday, spacecraft, corr=1)

In pygac 1.2 (CLARA-A2 L1C data) the scaled radiance output was corrected for changing Sun-Earth-distance, but not anymore in the updated pygac Version for the test CLARA-A3 L1C data.

@carloshorn
Copy link
Collaborator Author

Last thing to do are tests:

  1. Test if a changed json file leads to the desired warning (md5 hash test) and produces the expected results.
  2. Test if custom coefficients work.
  3. Test if the solar calibration throws the deprecation warning if the argument corr is used.

Anything I forgot?

@carloshorn
Copy link
Collaborator Author

I implemented the tests into test_calibrate_klm.py even if they are not klm specific. Are you okay with it, or should I move them into a separate calibration coefficient specific test case, something like test_calibration_coefficients.py?

@carloshorn
Copy link
Collaborator Author

@sfinkens, you once mentioned that in order to use coefficients of older PyGAC versions, you cherry-picked them from previous commits. If wanted, you could tell me which commits you used, and which version tag I should give them, so I can produce json files for them and add them together with their md5 hash.
I could also provide a tool to convert PATMOS-x coefficients into PyGAC coefficients so we can build internal version tags for all of them, but I guess this is beyond the scope of this PR.

@sfinkens
Copy link
Member

sfinkens commented Jun 9, 2020

@carloshorn That's very kind of you, but there is no need to do this for our past activities. That's frozen anyway. But for the future it will be very helpful!

Haven't looked at the tests in detail, buy your proposal sounds good. And a separate test case is a good choice in my opinion!

@mraspaud
Copy link
Member

mraspaud commented Jun 9, 2020

Great work @carloshorn ! I agree with @sfinkens for both points.

@carloshorn
Copy link
Collaborator Author

carloshorn commented Jun 9, 2020

Alright, I hope that's it. In summary:
With this PR, the calibration coefficients will no longer be a static part of calibration.py. The default values are stored in the external file data/calibration.json, which can be used as template for user defined defaults. The user can add his/her defaults through the PyGAC configuration file, e.g.

[calibration]
coeffs_file = /path/to/user/default/coeffs.json

If the user passes an official PyGAC defaults file (currently there is only one), the code will recognise it. At runtime, it is possible to check the calibration file version using the class method Calibrator.get_version().

Furthermore, it is possible to change individual coefficients by passing the satellite specific changes to the reader class using the keyword calibration in the constructor, e.g.

my_calibration= {"channel_1": {"dark_count": 43.21}}
filepath = "/path/to/file"
Reader = get_reader_class(filepath)
reader = Reader(calibration=my_calibration)
reader.read(filepath)

In addition, the functions of calibration.py will have more comment lines explaining each step and giving references to the applied methods, which should be beneficial for the readability and future development.

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

Very nice, I can't find anything but a single typo 😄 And so much documentation for the calibration! Love it!

pygac/calibration.py Outdated Show resolved Hide resolved
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, thanks for making the coeffs configurable!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mraspaud mraspaud merged commit 3162edd into pytroll:master Jun 11, 2020
@carloshorn carloshorn deleted the coefficients branch July 14, 2020 11:00
@sfinkens sfinkens mentioned this pull request Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Computation of Earth scene radiance
6 participants