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 avhrr instrument naming #136

Merged
merged 25 commits into from
Oct 11, 2022

Conversation

adybbroe
Copy link
Collaborator

@adybbroe adybbroe commented Dec 14, 2021

Adam.Dybbroe added 2 commits December 14, 2021 21:59
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
doc/conf.py Outdated
@@ -106,7 +106,7 @@ def __getattr__(cls, name):

# General information about the project.
project = u'Pyspectral'
copyright = u'2013-2018, PyTroll'
copyright = u'2013-2021, Pytroll'
Copy link
Member

Choose a reason for hiding this comment

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

Depending on how bored you are you could make this use the current year by importing datetime and doing:

Suggested change
copyright = u'2013-2021, Pytroll'
copyright = f'2013-{datetime.utcnow():%Y}, Pytroll'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my emacs I have it so that when I edit a file it will update the year automatically. So I am okay with how it is.

Copy link
Member

Choose a reason for hiding this comment

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

But that requires you to edit this file and it isn't about what works for you, it's what works for all contributors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, yes, I agree. Can we defer this to a more general discussion on how we should handle the headers in all Pytroll code? I rather don't want to change this now in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why it needs to be discussed. Plus this year in this PR is already wrong as it should probably be 2022, right? As for other pytroll code, pyresample and satpy both do the equivalent:

https://github.com/pytroll/pyresample/blob/main/docs/source/conf.py#L67

https://github.com/pytroll/satpy/blob/main/doc/source/conf.py#L116

@djhoese
Copy link
Member

djhoese commented Sep 28, 2022

@adybbroe what is the state of this PR?

@adybbroe
Copy link
Collaborator Author

@adybbroe what is the state of this PR?

Very good question. Will try to find out...

@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #136 (2ace905) into main (3a0f406) will increase coverage by 0.17%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
+ Coverage   88.90%   89.08%   +0.17%     
==========================================
  Files          22       22              
  Lines        2506     2529      +23     
==========================================
+ Hits         2228     2253      +25     
+ Misses        278      276       -2     
Flag Coverage Δ
unittests 89.08% <96.66%> (+0.17%) ⬆️

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

Impacted Files Coverage Δ
pyspectral/rsr_reader.py 77.55% <84.61%> (-0.44%) ⬇️
pyspectral/tests/test_rsr_reader.py 100.00% <100.00%> (ø)
pyspectral/tests/test_utils.py 98.00% <100.00%> (+0.11%) ⬆️
pyspectral/utils.py 90.53% <100.00%> (+0.53%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@adybbroe adybbroe marked this pull request as ready for review September 28, 2022 14:18
@adybbroe adybbroe self-assigned this Sep 29, 2022
pyspectral/rsr_reader.py Outdated Show resolved Hide resolved
adybbroe and others added 5 commits October 5, 2022 16:47
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
# Conflicts:
#	pyspectral/rsr_reader.py
#	pyspectral/tests/test_utils.py
#	pyspectral/utils.py
…equire one of platform_name/instrument or filename

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…pectral into fix-avhrr-instrument-naming

# Conflicts:
#	pyspectral/rsr_reader.py
#	pyspectral/utils.py
@@ -284,13 +286,13 @@ def download_rsr(dest_dir=None, dry_run=False):
functions from the internet as tarballs, extracts them, then deletes
the tarball.

See :func:`pyspectral.rsr_reader.check_and_download` for a "smart" version
See: func: `pyspectral.rsr_reader.check_and_download` for a "smart" version
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what did this, but this formatting is wrong. func is supposed to be surrounded by the colons and right next to the backtick of the function path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What a mess, I think I have reverted back to how it was now. Thanks for spotting this!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, weird. I wouldn't think flake8 would do this and isort shouldn't be touching docstrings.

of this process that only downloads the necessary files.

Args:
dest_dir (str): Path to put the temporary tarball and extracted RSR
dest_dir(str): Path to put the temporary tarball and extracted RSR
Copy link
Member

Choose a reason for hiding this comment

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

Also not sure if this was automated or not, but I'm pretty sure it is required/standard that there be a space between the argument name and data type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really weird! That must have been automated. And be due to the pre-commit stuff.

pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
pyspectral/utils.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Adam.Dybbroe added 4 commits October 10, 2022 17:12
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
…hrr-3'

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>
# Conflicts:
#	pyspectral/tests/test_utils.py
#	pyspectral/utils.py
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, just one unnecessary line in the pre-commit config.

@@ -6,3 +6,12 @@ repos:
hooks:
- id: flake8
additional_dependencies: [flake8-docstrings, flake8-debugger, flake8-bugbear]
args: [--max-complexity, "10"]
Copy link
Member

Choose a reason for hiding this comment

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

this is only needed when the mccabe plugin is requested I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, thanks!

.pre-commit-config.yaml Outdated Show resolved Hide resolved
Not needed. Only needed when the mccabe plugin is requested we believe
@adybbroe adybbroe merged commit 1f4e580 into pytroll:main Oct 11, 2022
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.

AVHRR instrument naming - allow avhrr-#
3 participants