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

Usgs Astro CSM Code Changes #30

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

acpaquette
Copy link

Hello,

Here are some changes from the USGS astro fork of the CSM. Most of these changes were made for CSM 3.0.3 but may be portable into the current version. The most notable are the CMake files and potentially the CI for appveyor and travis.

I just made the PR into the master area but I can update the PR as needed.

Adam Paquette

@sminster
Copy link
Collaborator

sminster commented Jan 9, 2024

Some initial thoughts:

  • Several of the changes reference "conda". I'm not sure that depending on Anaconda (or miniconda) for this library is appropriate.
  • The changes (and the PR title) refer to 3.0.3. The latest version of CSM is 3.1.0. This PR needs to be updated for those latest changes.
  • The changes in Ellipsoid.h just seem to be whitespace changes. They should probably be removed.
  • I don't like how Ellipsoid.cpp was changed to have Windows line endings (CRLF). All the other files have UNIX line endings (LF). The changes other than the whitespace changes in that file remove changes that were previously done that I don't think we want undone.

I'll probably have other comments, these are just my initial thoughts.

@acpaquette
Copy link
Author

Sweet, thank you for the initial review @sminster. We initially used 3.0.3.3 to create the USGSCSM set of camera so I wasn't sure if we would want the updates in 3.0.3 or main. I can rebase onto main and make some of the changes you have suggested

@acpaquette acpaquette changed the title Usgs Astro CSM Code Changes (CSM 3.0.3) Usgs Astro CSM Code Changes Jan 9, 2024
@acpaquette
Copy link
Author

acpaquette commented Jan 31, 2024

@sminster I think I addressed most of your initial thoughts, and this PR should be ready for a review

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.

5 participants