Skip to content

Added a fix to convert to default CRS before storing bounding box stats.#308

Merged
chrisbennight merged 1 commit intomasterfrom
GEOWAVE-305
Apr 3, 2015
Merged

Added a fix to convert to default CRS before storing bounding box stats.#308
chrisbennight merged 1 commit intomasterfrom
GEOWAVE-305

Conversation

@jwomeara
Copy link
Copy Markdown
Contributor

@jwomeara jwomeara commented Apr 3, 2015

Hi

@jwomeara
Copy link
Copy Markdown
Contributor Author

jwomeara commented Apr 3, 2015

Somebody want to check this out for me?

I updated the code to automatically convert entries to our default CRS when recording bounding box stats for feature data. I added a test for this as well.

I also made a utility class for the reprojection method that we have been using in a few places. There are also a few random 'code cleanup' and 'formatter' changes.

@rwgdrummer
Copy link
Copy Markdown
Contributor

+1.
Looks good. Will wait till travis finish.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 31.85% when pulling 7e758ff on GEOWAVE-305 into f481485 on master.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know you were re-factoring existing code, but in the case of not being able to transform to EPSG:4326 do we really want to return an feature in the incorrect crs (presumably to be uploaded)? (Caveat: I didn't check upstream of this code to see if there's a CRS check)

I would think it would be better not to upload and throw an error in the log (or maybe even runtime exception, though leaning away from that). Probably last thing we want is to upload inconsistent info.

Or I could be crazy, thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The comments explain the original thought process a bit. It seems
reasonable to me though to log an error and cancel the operation when we
can't convert to the default CRS.

Want me to make a ticket or update the existing pull request?
On Apr 3, 2015 7:08 PM, "Chris Bennight" notifications@github.com wrote:

In
geowave-vector/src/main/java/mil/nga/giat/geowave/vector/util/FeatureDataUtils.java
#308 (comment):

  •               crs) && (transform != null)) {
    
  •           // we can use the transform we have already calculated for this
    
  •           // feature
    
  •           featureTransform = transform;
    
  •       }
    
  •       else if (crs != null) {
    
  •           // this feature differs from the persisted type in CRS,
    
  •           // calculate the transform
    
  •           try {
    
  •               featureTransform = CRS.findMathTransform(
    
  •                       crs,
    
  •                       GeoWaveGTDataStore.DEFAULT_CRS,
    
  •                       true);
    
  •           }
    
  •           catch (final FactoryException e) {
    
  •               LOGGER.warn(
    

I know you were re-factoring existing code, but in the case of not being
able to transform to EPSG:4326 do we really want to return an feature in
the incorrect crs (presumably to be uploaded)? (Caveat: I didn't check
upstream of this code to see if there's a CRS check)

I would think it would be better not to upload and throw an error in the
log (or maybe even runtime exception, though leaning away from that).
Probably last thing we want is to upload inconsistent info.

Or I could be crazy, thoughts?


Reply to this email directly or view it on GitHub
https://github.com/ngageoint/geowave/pull/308/files#r27762036.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment only helps if people read it. Whoops.
New ticket is fine - I can make one
#309

chrisbennight added a commit that referenced this pull request Apr 3, 2015
Added a fix to convert to default CRS before storing bounding box stats.
@chrisbennight chrisbennight merged commit 39bbc19 into master Apr 3, 2015
@rfecher rfecher deleted the GEOWAVE-305 branch May 5, 2015 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants