compare_dswx_hls_products and dependencies gathered into new standalo… #213
Conversation
There was a problem hiding this comment.
All of the issues with the actual comparisons are addressed well.
Every comparison has a "status" line starting with '[OK]. ', '[FAIL] ', or '[INFO] ', which is good. When the comparison fails, we should be consistent about where the additional info goes - either always before or always right after (and before reporting failure details, it should say what is being compared). I suggest having it go right after, as that saves a line identifying what is being reported on. I made some specific suggestions in-line with the code to address this point.
These changes might seem like nit-picking, but since this will (hopefully) be used by multiple teams, and think having really clear and intuitive output is important.
I attached two text files showing what the output looks like when a comparison succeeds vs fails, to see a concrete example of what I'm commenting on. The failed output was on two different granules, so pretty much everything was wrong.
comparison_failed_output.txt
comparison_succeeded_output.txt
| # Don't fail for metadata fields that are not required to be the same | ||
| if k1 in metadata_exclude_list: | ||
| # We will just print the difference in the output | ||
| print(f'[INFO] {msg}') |
There was a problem hiding this comment.
These will print before the "Comparing metadata" message on line 140, which might be confusing. I suggest just appending these into the metadata_error_message string, or making another return argument to contain the info messages, and print them all at the same time. Probably good to group these together after the other metadata error messages.
There was a problem hiding this comment.
I am committing changes to address the most-recent comments.
Here is sample output for a few scenarios with the new changes:
comparison_all_ok.txt
comparison_band_difference_fail.txt
| flag_same_nbands = nbands_1 == nbands_2 | ||
| flag_all_ok, flag_same_nbands_str = _get_prefix_str(flag_same_nbands, flag_all_ok) | ||
| prefix = PREFIX | ||
| print(f'{flag_same_nbands_str}Comparing number of bands') |
There was a problem hiding this comment.
Add the prefix at the beginning of this line, to keep the first 7 characters of each line clear of anything other than a comparison status.
Or you can remove it. If you put the error messages after the "[FAIL] Band 1 - Water classification (WTR)" line, you don't need this one.
|
|
||
| flag_all_ok, flag_bands_are_equal_str = _get_prefix_str(flag_bands_are_equal, | ||
| flag_all_ok) | ||
| print(f'{flag_bands_are_equal_str} Band {b} -' |
There was a problem hiding this comment.
This has some extra spaces that mis-aligns the "Band {b}" part of the message.
Given my comment up above about removing the "Comparing DSWx bands..." line, replace those spaces with "Comparing ", so that it reads like one of these:
[FAIL] Comparing Band 1 - Water classification (WTR)
[OK] Comparing Band 1 - Water classification (WTR)
At the very least, remove the extra extra spaces, so that "Band {b}" aligns with the rest of the output.
| flag_all_ok, flag_bands_are_equal_str = _get_prefix_str(flag_bands_are_equal, | ||
| flag_all_ok) | ||
| print(f'{flag_bands_are_equal_str} Band {b} -' | ||
| f' {gdal_band_1.GetDescription()}"') |
There was a problem hiding this comment.
Extra double-quote at the end of the printed line
There was a problem hiding this comment.
Thanks for all the hard work on this @david-inglish, and thanks again to @sjlewis-jpl for weighing in!
…ne script
Description
Develop a PGE version of the compare_dswx_hls_products script to enable customization for PGE purposes.
Affected Issues
#208
Testing
Tested with output files from different releases