-
Notifications
You must be signed in to change notification settings - Fork 7
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
DISP-S1 Final 0.5.3 SAS integration #565
Conversation
2 removed identification fields Many many new fields missing official documentation Commented out stubs added where documentation is missing
New fields updated from code Descriptions sourced from code DTypes from inspecting output files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but had some issues with the measured parameters config additions
metadata/pge_runconfig: | ||
attribute_data_type: string | ||
attribute_type: processingInformation | ||
description: The full PGE runconfig YAML file used to generate the product. | ||
display_name: PGERunConfig | ||
metadata/algorithm_parameters_yaml: | ||
attribute_data_type: string | ||
attribute_type: processingInformation | ||
description: The full algorithm parameters YAML file used to generate the product. | ||
display_name: AlgorithmParametersYAML | ||
metadata/dolphin_workflow_config: | ||
attribute_data_type: string | ||
attribute_type: processingInformation | ||
description: The configuration parameters used by `dolphin` during the processing. | ||
display_name: DolphinWorkflowConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry a bit about including these wholesale within the ISO xml. I could see it introducing subtle XML parsing errors based on what is in the configs we include, so I think better to just leave this section out entirely.
metadata/ceos_noise_removal: | ||
attribute_data_type: string | ||
attribute_type: processingInformation | ||
description: Flag if noise removal* has been applied (Y/N). Metadata should include the noise removal algorithm and reference to the algorithm as URL or DOI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was inclusion of the * here intentional or a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copy pasted from the DISP code which does have the *
I'll remove it
metadata/ceos_persistent_scatterer_amplitude_dispersion_threshold: | ||
attribute_data_type: float | ||
attribute_type: processingInformation | ||
description: "TBA" #TODO: ask Scott S - this is a duplicate description in code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "TBA" as a placeholder here, lets just reiterate the name of the variable: description: CEOS persistent scatterer amplitude dispersion threshold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is also good. https://github.com/opera-adt/disp-s1/pull/252/files would be the way I'd correct it, but these changes are no better than your reiteration
@collinss-jpl requested changes have been made |
Description
Affected Issues
Testing