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.

Sign up for GitHub

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

Jump to bottom

Add option to specify sun-zenith angle threshold applied #108

Merged
merged 4 commits into from
Jun 23, 2020
Merged

Add option to specify sun-zenith angle threshold applied #108

merged 4 commits into from
Jun 23, 2020

Conversation

adybbroe
Copy link
Collaborator

@adybbroe adybbroe commented May 6, 2020 •
edited
Loading

Add option to specify sun-zenith angle threshold in the 3.x emissive/reflectance separation, and set default to 85 (instead of 88)

  • Closes #xxxx
  • Tests added
  • Tests passed: Passes pytest pyspectral
  • Passes flake8 pyspectral
  • Fully documented

Add option to specify sun-zenith angle threshold in the 3.x emissive/…
e9a234f 
…reflectance separation,

and set default to 85 (instead of 88)

Signed-off-by: Adam.Dybbroe <[email protected]>
@adybbroe adybbroe self-assigned this May 6, 2020
@adybbroe
Copy link
Collaborator Author

adybbroe commented May 6, 2020

I will try have a look at adding some tests, shortly

@coveralls
Copy link

coveralls commented May 6, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 71.403% when pulling 9f527fd on adybbroe:emissive-reflectance-separation-sunz-threshold into 2d2da5a on pytroll:master.

@adybbroe adybbroe requested review from mraspaud and pnuu May 6, 2020 16:44
pnuu
pnuu approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@pnuu pnuu 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 question inline.

pyspectral/near_infrared_reflectance.py Show resolved Hide resolved
mraspaud
mraspaud approved these changes May 7, 2020
View reviewed changes
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, I just have a style comment in here.

pyspectral/near_infrared_reflectance.py Outdated Show resolved Hide resolved
Adam.Dybbroe added 2 commits May 8, 2020 14:49
@adybbroe
Copy link
Collaborator Author

adybbroe commented May 8, 2020

@mraspaud I made the few key word arguments explicit, but now I get complaints from Codacy about too many arguments, see above

@mraspaud
Copy link
Member

mraspaud commented May 8, 2020

Ok. So maybe some arguments don't need to be passed to init, but can be passed to the method later on ? (I haven't looked at the code though, so I don't know)
Other solution: bunch the arguments together in classes. Not ideal, but better than just kwargs I think.
Last option: 6 arguments is still ok I think, you can leave it like this.

@adybbroe adybbroe merged commit 34b012a into pytroll:master Jun 23, 2020
@adybbroe adybbroe deleted the emissive-reflectance-separation-sunz-threshold branch June 23, 2020 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraspaud mraspaud mraspaud approved these changes

@pnuu pnuu pnuu approved these changes

Assignees

@adybbroe adybbroe

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adybbroe @coveralls @mraspaud @pnuu