Skip to content
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

added projection code #524

Merged
merged 8 commits into from
Apr 3, 2023
Merged

added projection code #524

merged 8 commits into from
Apr 3, 2023

Conversation

Kelvinrr
Copy link
Collaborator

@Kelvinrr Kelvinrr commented Mar 29, 2023

Adds ability to add projections to usgscsm ISDs, part of the EIS projection support work.

@Kelvinrr Kelvinrr requested a review from acpaquette March 29, 2023 04:42
ale/base/base.py Outdated
Comment on lines 333 to 337
try:
from osgeo import gdal
except:
self._projection = None
return self._projection
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Localized the import to keep the optional gdal stuff in one location without complicating things by adding some gdal module or something.

Comment on lines +34 to +35
run_contrained:
- gdal
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will only install if it can do so without conflicts. important to co-install with ISIS.

Comment on lines +72 to +75
Group = Mapping
ProjectionName = Sinusoidal
CenterLongitude = 148.36859083039
TargetName = MARS
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just kinda shoved a projection into an MRO image for testing, I imagine the same to what a real image is like.

Copy link
Collaborator

@acpaquette acpaquette left a comment

Choose a reason for hiding this comment

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

Overall looks good, we need to get the geoTransform attached to the isd to finish this off.

We also need to define how those fields are accessed in ALE. I have written the functions to do so in USGSCSM, but will paste them here:

std::vector<double> getGeoTransform(json isd) {
  std::vector<double> transform = {};
  try {
    transform = isd.at("geo_transform").get<std::vector<double>>();
  } catch (std::exception &e) {
    std::string originalError = e.what();
    std::string msg = "Could not parse the geo_transform. ERROR: " + originalError + isd.dump();
    throw std::runtime_error(msg);
  }
  return transform;
}

std::string getProjectionString(json isd) {
  std::string projection_string = "";
  try {
    projection_string = isd.at("proj_string");
  } catch (...) {
    throw std::runtime_error("Could not parse the projection string.");
  }
  return projection_string;
}

ale/base/base.py Outdated Show resolved Hide resolved
ale/formatters/usgscsm_formatter.py Show resolved Hide resolved
try:
from osgeo import gdal
except:
self._geotransform = (0.0, 1.0, 0.0, 0.0, 0.0, 1.0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im preeeetty sure this is the identity, probably good as a default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that should identity for GDAL. https://github.com/OSGeo/gdal/blob/c642af0722d43bb337aaf86293129dbed576e534/alg/gdaltransformer.cpp#L1847

But just a warning, this is not same order for all packages.

@Kelvinrr
Copy link
Collaborator Author

Kelvinrr commented Mar 29, 2023

@acpaquette I think we are on the same page here except Im using geotransfmrm without underscore. And prog_string is projection. Projection is NULL if it doesn't exist, otherwise prog4 string. Maybe I can change it to something else easier to parse?

Otherwise geotransform being the identity when no transformation which I think makes sense.

Copy link
Collaborator

@acpaquette acpaquette left a comment

Choose a reason for hiding this comment

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

Overall looks good, one last comment to address

src/Util.cpp Outdated Show resolved Hide resolved
src/Util.cpp Outdated Show resolved Hide resolved
@Kelvinrr Kelvinrr merged commit 52e2081 into DOI-USGS:main Apr 3, 2023
jrcain-usgs pushed a commit to jrcain-usgs/ale that referenced this pull request May 19, 2023
* added projection code

* fixed test

* added geotransform

* addded indentity matrix

* added to generic ISD

* adding getters

* addressed comment

* more comment
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.

3 participants