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

Correct coefficients #67

Merged
merged 6 commits into from
Jun 18, 2020
Merged

Conversation

carloshorn
Copy link
Collaborator

This PR should correct all typos mentioned in #66 and also correct the the intercept coefficient mentioned in #57.

Therefore, it should close #57 and close #66.

Since the coefficients are currently coming from PATMOS-x, should I add the script to convert these tar balls to PyGAC json files to the repository?

@sfinkens
Copy link
Member

Sounds good!

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.

Nice work! Only two questions.

bin/pygac-convert-patmosx-coefficients Show resolved Hide resolved
pygac/data/calibration.json Show resolved Hide resolved
@sfinkens
Copy link
Member

@carloshorn So this is ready to merge?
@ninahakansson FYI, this PR will slightly change the calibrated data

@sfinkens sfinkens added the bug label Jun 18, 2020
@carloshorn
Copy link
Collaborator Author

@carloshorn So this is ready to merge?

Please note that the script requires python 3 (e.g. yield from is not valid in python 2), while PyGAC also works with python 2.
If this is okay, it is ready to merge.

@sfinkens
Copy link
Member

sfinkens commented Jun 18, 2020

I think that's fine, it's just a tool. But could you please raise an exception if someone tries to call it with python 2?

@carloshorn
Copy link
Collaborator Author

The example yield from already raises a SyntaxError using python 2. I could replace the first line with #/usr/bin/env/ python3, but I don't know if this has any side effects when running the setup.py.

@sfinkens
Copy link
Member

What about something more verbose for an unexperienced user? RuntimeError: This tool is Python 3 only or something like that?

@carloshorn
Copy link
Collaborator Author

Yes, that would be nice, but unfortunately, the syntax error is thrown while the python interpreter reads the script. I don't get to the point where I could implement it. Of cause, I could rewrite this line with for item in items: yield item, but I fear that something else pops up next and this will just be the first round of ugly python 2 backward compatibility rewriting....

@sfinkens
Copy link
Member

Oh sorry, now I see the problem. My bad! Then it's fine of course.

@sfinkens sfinkens merged commit 4e985e1 into pytroll:master Jun 18, 2020
@carloshorn carloshorn deleted the correct_coefficients branch July 14, 2020 11:00
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.

Typos in calibration coefficients channel 4 BT to radiance conversion
3 participants