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

CSM Camera Skymap #5072

Merged
merged 33 commits into from
Feb 9, 2024
Merged

CSM Camera Skymap #5072

merged 33 commits into from
Feb 9, 2024

Conversation

acpaquette
Copy link
Collaborator

@acpaquette acpaquette commented Nov 10, 2022

Attempt at getting the CSM camera to work with skymap

Description

These changes are related to getting the MSL Mastcam data projected using ISIS but met a hard block once the setSky function was hit. Within ISIS this uses the camera look vector to generate a focalplaneX/Y from a line/sample but the CSM doesn't support data within the camera reference frame.

There is still room for experimentation but it seems like we will need a specific program for creating projections from rover data.

Related Issue

Ground based sensor work (See USGSCSM IAA Proposal)

How Has This Been Validated?

Has not been validated as the changes are incomplete

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation change (update to the documentation; no code change)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Infrastructure change (changes to things like CI or the build system that do not impact users)

Checklist:

  • I have read and agree to abide by the Code of Conduct
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have added myself to the .zenodo.json document.
  • I have added any user impacting changes to the CHANGELOG.md document.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

  • I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

@acpaquette acpaquette changed the title CSM Camera Attempted Skymap (Book keeping -- avoid merge) CSM Camera Skymap May 8, 2023
@acpaquette acpaquette marked this pull request as ready for review May 8, 2023 23:07
isis/src/base/apps/csminit/csminit.cpp Outdated Show resolved Hide resolved
isis/src/base/apps/skymap/main.cpp Show resolved Hide resolved
isis/src/base/apps/skymap/main.cpp Show resolved Hide resolved
isis/src/base/apps/skymap/skymap.xml Outdated Show resolved Hide resolved
isis/src/base/objs/CSMCamera/CSMCamera.cpp Show resolved Hide resolved
isis/src/base/objs/CSMCamera/CSMCamera.cpp Outdated Show resolved Hide resolved
isis/src/base/objs/CSMSkyMap/CSMSkyMap.cpp Show resolved Hide resolved
AustinSanders
AustinSanders previously approved these changes Oct 3, 2023
Copy link
Contributor

@AustinSanders AustinSanders left a comment

Choose a reason for hiding this comment

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

I think this needs a changelog entry and then we're good to go

AustinSanders
AustinSanders previously approved these changes Oct 13, 2023
AustinSanders
AustinSanders previously approved these changes Oct 25, 2023
Copy link
Collaborator

@Kelvinrr Kelvinrr left a comment

Choose a reason for hiding this comment

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

I think we also need tests for the new CSMCamera stuff?

Comment on lines +40 to +41
- CSMCamera can now read and use the body rotation from ALE produced ISDs [#5072](https://github.com/DOI-USGS/ISIS3/pull/5072)
- CSMSkyMap added to CSMCamera for use with local rover projections in ISIS [#5072](https://github.com/DOI-USGS/ISIS3/pull/5072)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't normally allow more than one changelog but this is an old PR so we can probably let it slide.

isis/src/base/apps/csminit/csminit.cpp Outdated Show resolved Hide resolved
@acpaquette
Copy link
Collaborator Author

@Kelvinrr Tests are present, I will double check add potentially add some for coverage

@acpaquette acpaquette requested a review from Kelvinrr January 31, 2024 19:18
Kelvinrr
Kelvinrr previously approved these changes Jan 31, 2024
@Kelvinrr
Copy link
Collaborator

Kelvinrr commented Feb 5, 2024

skymap_app_test_user seems to be failing

Comment on lines +259 to +260
if (abs(p_incam->RightAscension() - lon) > 0.1) return false;
if (abs(p_incam->Declination() - lat) > 0.1) return false;
Copy link
Collaborator Author

@acpaquette acpaquette Feb 5, 2024

Choose a reason for hiding this comment

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

@Kelvinrr These lines were added to avoid dual projection, changing the values in the skymap test. Basically any look vector projects into two points in right ascension and declination, so if you project to 0, 0 you also project onto 0, 180. This checks avoids look vectors projecting into the inverse right ascension, declination

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you update the test data? We just got to make sure we aren't adding another failing test if we dont have to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated data, we usually check it in once the PR is merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

kk just making sure

@acpaquette acpaquette merged commit e417363 into DOI-USGS:dev Feb 9, 2024
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