NDCUM-1016

[mikedorfman]

• Updated call to updatePolygon so we're calling updatePolygons with a polygon array length of just the single polygon instead. This ensures we're using counter-clockwise arrangement of points in the polygon and reduces dual maintenance between these two methods.
• Updated updatePolygons so we can optionally bypass the polygon validity check for backwards compatibility with updatePolygon.
• Removed no-longer-used updatePolygon method.

[yenes56]

I have discussed with our lead @skorper
and we would like invalidOK to be removed. The old addPolygon function does not check if polygon is valid neither did it make sure coordinates are counter clockwise. if the polygon was not valid, then it would probably fail CMR Post anyway. Hence, please remove invalidOK and all calls to addPolygons shall go through

• validation
• counter clockwise-zed

if validation failed, global bounding box is added as geometry presentation, that logic is in addPolygons already.

thanks!

[yenes56]

Please use apache lang3 StringUtils.isEmpty to accomplish this very same logic. It is null proof.
Hopefully not string with only space but it could be " "

hence. StringUtils.isEmpty(StringUtils.trim(polygonString)) will do

I like the way the variable is named. Thumb up on that

[mikedorfman]

Sounds good - updated in the next commit.

[mikedorfman]

Hey @yenes56 - thanks for taking a look! That makes sense. What are your thoughts on attempting Polygon repair in the script if it's invalid? Right now, we have a polygon that does not pass this validity check but still publishes to CMR successfully. I'll take a closer look into why it's invalid to see if there's anything we can do elsewhere to fix this.

[mikedorfman]

@yenes56 - following up on this, I figured out why my validity check is failing - the polygon spans the International Date Line and, because of its geometry, it overlaps itself. This is not considered invalid in CMR, but it doesn't pass the validity test here. Would you still like the global bounding box to be used in this case?

[mikedorfman]

An alternative thought @yenes56 - what are your thoughts on removing polygon validation and instead making a (optional) call to the CMR validate-granule endpoint (https://cmr.earthdata.nasa.gov/ingest/site/docs/ingest/api.html#validate-granule-endpoint) after the UMM-G is generated? That would assure the validation run against the generated UMM-G file is exactly what's run on CMR (assuming validation run here should, by definition, match the validation run by CMR). The downside is that it would add an external dependency, but it would remove the dual maintenance for validity checks between CMR and the MetadataAggregator.

[skorper]

@mikedorfman I like your ideas about polygon repair and CMR validation. Do you mind if we address/discuss those in a separate ticket and merge this as-is? I think this looks good and we can proceed with these changes

[mikedorfman]

@skorper - sounds good to me - thanks!

[mikedorfman]

@skorper - just following up on this - we're currently maintaining our own branch of the cumulus-metadata-aggregator with both the polygon-validity updates (changes in this PR) and a fix for the IDL-crossing case (mentioned needs more discussion above) and we've merged the latest Cumulus-V16-compatible release into our fork. We had a hard time with the merge because of the number of changes, so for the time being we're sticking with the previous version (our fork) of the cumulus-metadata-aggregator - which seems to be working in Cumulus v16 and Cumulus v18.

Are either of the changes above currently in the cumulus-v16-compatible release? How would you like to proceed WRT the IDL-crossing case? I'm happy to make a new PR where we can continue discussion if that works for you.

[skorper]

Hi @mikedorfman, the reason you probably had trouble merging our latest release is because we recently implemented something similar to support SWOT.

cumulus-metadata-aggregator:8.5.0 has a new feature where polygons extracted from the .iso.xml will be split into multiple polygons if they cross the IDL. It sounds like this is similar to the behavior you've added in this PR -- I wonder if you can try this capability out on our deployment and see if it works for you?

If not, we can take a look at merging your code into ours, we definitely want to help get your changes merged in so you don't have to work on a separate fork. Thank you!

[mikedorfman]

@skorper - sounds good - we'll give it a try just referencing the podaac release and will let you know how it goes! We'll probably give this a shot in the next week or two - I'll keep you updated.