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

Facilitate retrieval of CTP even if cloud shadow is not computed #33

Conversation

heptaflar
Copy link
Collaborator

This pull request would enable the retrieval of CTP on request, even if cloud shadow is not requested. This makes sense in cases where one needs CTP in subsequent processing steps but does not need cloud shadow or is going to compute cloud shadow by different means, which need CTP as input.

@dolaf
Copy link
Collaborator

dolaf commented Jan 28, 2021

looks ok for me

Copy link
Collaborator

@TonioF TonioF left a comment

Choose a reason for hiding this comment

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

In general, this seems to work fine (and it makes more sense than the existing implementation). Three comments:

  1. The parameter outputCtp is not well named anymore. The import part is that it is computed, so it should be "computeCtp". The renaming probably would cause backwards incompatibility, though, so I don't insist on this.
  2. I guess it would be okay to output the ctp band everytime it is computed, either because "outputCtp" or "computeCloudShadow" is true.
  3. I'd love if in the label and the description you could use "cloud top pressure" instead of CTP.

@marpet
Copy link
Contributor

marpet commented Jan 29, 2021

In the @parameter annotation alias="computeCtp" could be added. Then outputCtp will still work, but computeCtp has precedence.

@marpet
Copy link
Contributor

marpet commented Jan 29, 2021

Is this applicable to other sensors besides OLCI? Just to harmonise things.

@marpet marpet added enhancement New feature or request OLCI S3 OLCI Idepix labels Jan 29, 2021
@heptaflar
Copy link
Collaborator Author

I agree with the first and the last point Tonio raised. Renaming the parameter to "computeCtp" while introducing an alias "outputCtp" ensures compatibility with previous versions. Thanks Marco for pointing this out.

I am not sure about Tonio's second point, however. For me, cloud shadow and CTP are separate concerns. I prefer the present behaviour, i.e., CTP is only put out, if requested explicitly. Anyway, Tonio's suggestion would work for me, too.

I modified the original pull request to address point 1 and 3 on Tonio's list.

@TonioF
Copy link
Collaborator

TonioF commented Jan 29, 2021

I am not sure about Tonio's second point, however. For me, cloud shadow and CTP are separate concerns. I prefer the present behaviour, i.e., CTP is only put out, if requested explicitly. Anyway, Tonio's suggestion would work for me, too.

Any more thoughts on this? Then we can have a majority vote. I'd be fine with accepting now.

@marpet
Copy link
Contributor

marpet commented Jan 29, 2021

I would vote for Ralfs solution and leaving CTP and cloud shadow independent.

@dolaf
Copy link
Collaborator

dolaf commented Jan 29, 2021 via email

@heptaflar
Copy link
Collaborator Author

heptaflar commented Feb 2, 2021

Accepted by me. Who merges?

@marpet
Copy link
Contributor

marpet commented Feb 3, 2021

Probably Olaf, when he finds the time to do it

@marpet
Copy link
Contributor

marpet commented Mar 18, 2021

Not needed anymore. CTP calculation is not the purpose of IdePix. The output is only included for debugging purpose.

@marpet marpet closed this Mar 18, 2021
@marpet marpet deleted the facilitate-ctp-retrieval-without-cloud-shadow-computation branch July 19, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request OLCI S3 OLCI Idepix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants