Geostationary -- Calculate semi_minor_axis or inv_flattening

[rschmunk]

If either of semi_minor_axis or inv_flattening is present in the grid_mapping attributes but other is not, then the missing value can be calculated from the other and from the semi_major_axis.

[cwardgar]

In the event that both semi_minor_axis and inv_flattening are present, your final else block will be executed. Is that your intent?

You've got 4 possible outcomes based on the presence of the two variables. To me, it's clearer to list them explicitly, e.g.:

      if (Double.isNaN(semi_minor_axis) && Double.isNaN(inv_flattening)) {
         throw new IllegalArgumentException("Must specify "+CF.SEMI_MINOR_AXIS+" or "+CF.INVERSE_FLATTENING);
      } else if (Double.isNaN(semi_minor_axis)) {
          final double flattening = 1. / inv_flattening;
          semi_minor_axis = semi_major_axis * (1. - flattening);
      } else if (Double.isNaN(inv_flattening)) {
        if (semi_minor_axis != semi_major_axis) {
          final double flattening = (semi_major_axis - semi_minor_axis) / semi_major_axis;
          inv_flattening = 1. / flattening;
        }
        // Do nothing if semi_minor_axis == semi_major_axis, as that would cause: inv_flattening = 1. / 0.
      } else {
        assert !Double.isNaN(semi_minor_axis) && !Double.isNaN(inv_flattening);
        // Optional block.
      }

[cwardgar]

Updated code snippet. Also, should it be an error if we can't calculate inv_flattening due to division by zero?

[rschmunk]

My bad about mishandling the case of both being present. (Gives self head slap.)

I wasn't quite sure what to do about the case of semi_minor = semi_major, or whether anything needed to be done at all. If that condition occurs, then inv_flattening would have already been set to NaN above. One could argue it should instead be reset to POSITIVE_INFINITY, but does it matter?

The cases I have run into for using the geostationary projection have all been "real" data and have specified the spheroid attributes. It would be more than a bit odd for such data to specify data on the sphere (with semi_minor = semi_major) rather than a spheroid.

What's the easiest way for me to correct the code in my pull request?

[cwardgar]

In that case, I think that emitting an error if semi_minor == semi_major is reasonable. Or maybe just an assertion?

assert semi_minor != semi_major : "Should never happen with real data!";

Any change you make to the msdsoftware:master branch will automatically be incorporated into this PR. So go ahead and just add another commit. Then, if you want a cleaner history, you can do an interactive rebase. It should all get picked up here.

[rschmunk]

I've updated the code.

I left the case of semi-minor == semi_major as "do nothing" -- i.e., leave inv_flattening = NaN -- due to the possibility that someone might generate a sample dataset using the sphere. A very slim possibility but it's there.

Also, throwing an exception would block the dataset from being acquired, and I'm already running into projected grid datasets elsewhere that cannot be acquired/opened because grid mapping projection attributes are under- or poorly specified (cf #907).

[rschmunk]

@cwardgar, Travis seems to have borked building after I posted that code fix, but I can't tell whether it's something I got wrong in the code or if it's something else. It builds fine with Gradle on a local copy.

[cwardgar]

It's some sort of weird error in Travis's JVM v7. That's not our fault.

Can you close and reopen this PR? That should force Travis to rerun the checks.

[dopplershift]

You can restart Travis from the web UI, I just did it.

[cwardgar]

This seems to be a problem with Travis. Here is the last successful Travis build for the master branch:
https://travis-ci.org/Unidata/thredds/jobs/269734167

Here is a build of the same commit that I reran just a couple hours ago, now failing:
https://travis-ci.org/Unidata/thredds/jobs/269734167

The difference? java version "1.7.0_75" vs java version "1.7.0_121". Looks like Travis updated its JDK and things broke. I'll look more into this later. I wonder if the build would break using a local copy of 1.7.0_121?

[cwardgar]

I added a commit in #919 and then merged it into this branch. Fixed! So this is good to go now.