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

Mgs time bias #538

Merged
merged 4 commits into from
Jun 5, 2023
Merged

Mgs time bias #538

merged 4 commits into from
Jun 5, 2023

Conversation

acpaquette
Copy link
Collaborator

Address a time bias in the MGS NAC/WAC images resulting in poorly corresponding ephemeris data when compared to ISIS

@@ -140,7 +140,7 @@ def test_custom_frame_chain_iak(self, from_spice):
target_frame_id.return_value = -800
frame_chain = self.driver.frame_chain
assert len(frame_chain.nodes()) == 0
from_spice.assert_called_with(center_ephemeris_time=2.4, ephemeris_times=[2.4], nadir=False, sensor_frame=14082360, target_frame=-800, exact_ck_times=True)
from_spice.assert_called_with(center_ephemeris_time=2.4, ephemeris_times=[2.4], nadir=False, sensor_frame=14082360, target_frame=-800, exact_ck_times=True, inst_time_bias=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only necessary on cassini drivers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cassini has a special additional frame that is added to the end of the frame chain. It seems like that driver is the only thing testing the from_spice call.

I think that the NaifData class tests should have caught this as well, it's just hard to get all the checks.

@AustinSanders
Copy link
Contributor

AustinSanders commented May 24, 2023

Overall looks good! Are we planning on reenabling mgs drivers when this is merged, or leaving them out until this is 100% squared away?

Looks like this is already done!

AustinSanders
AustinSanders previously approved these changes May 25, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #538 (983feaa) into main (759b307) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
- Coverage   17.03%   17.00%   -0.03%     
==========================================
  Files          53       53              
  Lines        5824     5834      +10     
==========================================
  Hits          992      992              
- Misses       4832     4842      +10     
Impacted Files Coverage Δ
ale/base/base.py 0.00% <ø> (ø)
ale/base/data_naif.py 0.00% <0.00%> (ø)
ale/drivers/__init__.py 0.00% <ø> (ø)
ale/drivers/mgs_drivers.py 0.00% <0.00%> (ø)
ale/transformation.py 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acpaquette acpaquette merged commit 3a1b59e into DOI-USGS:main Jun 5, 2023
acpaquette added a commit to acpaquette/ale that referenced this pull request Jun 5, 2023
* Updated disclaimer for release

* Handle MGS time bias for NAC and WAC

* Re-enabled MGS drivers

* Updated changelog
acpaquette added a commit that referenced this pull request Jun 6, 2023
* Updated disclaimer for release

* Handle MGS time bias for NAC and WAC

* Re-enabled MGS drivers

* Updated changelog
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