Import for cassini iss fring mosaics rsfrench2025#1437
Open
Import for cassini iss fring mosaics rsfrench2025#1437
Conversation
…t_all_internal.sh, config_bundle_info.py, import_for_tests.sh, and import_util.py
…25 in config_bundle_info.py to include additional global mosaic files.
…update config_bundle_info.py to correct class name casing and primary index order for Cassini ISS F-ring mosaic bundle.
…al_def_product_types.json, including various image and metadata formats, as well as documentation and index files.
…_opus_import_volumes.sh for import and deployment.
…Files and ObsPds models by increasing field lengths for category, short_name, and primary_lid. Update related JSON schemas to reflect these changes. Adjust import logic to allow for missing shelf files in PDS4 products.
…-iss-fring-mosaic-iss_183ri_spokemov001_prime, and co-iss-n1479208692
…ta_api.py and add new product type entries for Cassini ISS F-ring mosaics in api_product_types_empty_search.json.
…lesser pixels size endpoints, and add new product type entry for Cassini ISS F-ring mosaics in api_help_bundles.html.
…ding camera property, updating image pixel size methods, and modifying instrument-related methods to return None where applicable. Adjust JSON schema for obs_ring_geometry to change form type from distance_resolution to generic_angle for projected longitudinal resolution fields.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds full support for the Cassini ISS F‑Ring mosaics bundle Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@opus/application/test_api/responses/api_product_types_empty_search.json`:
- Around line 1752-1759: The JSON object ending with "product_type":
"coiss_f_ring_browse_reproj_img_full" and "version_number": 999999 has its
closing brace `},` mis-indented; fix the indentation so the closing brace aligns
with the opening brace of that object (i.e., indent the `},` to the same level
as the opening `{` for that object) to maintain consistent JSON object structure
and valid formatting.
In `@opus/import/obs_bundle_cassini_iss_fring_mosaics_rsfrench2025.py`:
- Around line 37-39: The camera property accesses
self._index_col('min_image_name')[-1] without checking for None, which can raise
TypeError; update the camera property (camera) to first assign the result of
self._index_col('min_image_name') to a local variable, check if it is None or
empty, and return None (or a safe default) instead of indexing into None, and
ensure callers handle None; also update the related wavelength lookup code (the
methods using _COISS_FILTER_WAVELENGTHS at lines ~250-260) to guard against
camera being None (e.g., skip the lookup or handle a None key) to avoid KeyError
when using (None, 'CL1', 'CL2').
- Around line 250-260: The field_obs_wavelength_wavelength1 and
field_obs_wavelength_wavelength2 methods assume _COISS_FILTER_WAVELENGTHS has an
entry for (self.camera, 'CL1', 'CL2'); add a defensive validation that
self.camera is one of the expected values (e.g., 'N' or 'W') and/or that the
tuple key exists in _COISS_FILTER_WAVELENGTHS before indexing it, and if not
present raise a clear ValueError or return a safe fallback; ensure the check
references self.camera and the key (self.camera, 'CL1', 'CL2') so the error
message identifies the missing camera/key and the method names in the message
for easier debugging.
In `@opus/import/table_schemas/obs_ring_geometry.json`:
- Line 1036: The pi_form_type entry for the longitudinal resolution fields was
incorrectly set to use an angular unit suffix ("generic_angle"), which conflicts
with their distance semantics; edit the pi_form_type value for the affected
entries (the one currently showing "RANGE%.5f:generic_angle" and the similar
entry at the other occurrence) to use the distance unit suffix used by
RINGGEOprojectedlongitudinalresolution* (i.e., replace ":generic_angle" with the
correct distance unit suffix consistent with other
RINGGEOprojectedlongitudinalresolution fields so the API treats these as
distances).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f14bdb26-6cc1-4983-947a-332e02a44031
📒 Files selected for processing (27)
opus/application/apps/search/models.pyopus/application/test_api/create_test_results.pyopus/application/test_api/responses/api_help_bundles.htmlopus/application/test_api/responses/api_product_types_empty_search.jsonopus/application/test_api/responses/results_co_iss_fring_mosaic_iss_00ari_spkmovper001_prime_files.jsonopus/application/test_api/responses/results_co_iss_fring_mosaic_iss_00ari_spkmovper001_prime_images.jsonopus/application/test_api/responses/results_co_iss_fring_mosaic_iss_00ari_spkmovper001_prime_metadata.jsonopus/application/test_api/responses/results_co_iss_fring_mosaic_iss_183ri_spokemov001_prime_files.jsonopus/application/test_api/responses/results_co_iss_fring_mosaic_iss_183ri_spokemov001_prime_images.jsonopus/application/test_api/responses/results_co_iss_fring_mosaic_iss_183ri_spokemov001_prime_metadata.jsonopus/application/test_api/responses/results_co_iss_n1479208692_files.jsonopus/application/test_api/responses/results_co_iss_n1479208692_images.jsonopus/application/test_api/responses/results_co_iss_n1479208692_metadata.jsonopus/application/test_api/test_metadata_api.pyopus/import/_import_all_internal.shopus/import/config_bundle_info.pyopus/import/do_import.pyopus/import/import_for_tests.shopus/import/import_util.pyopus/import/main_opus_import.pyopus/import/obs_bundle_cassini_iss_fring_mosaics_rsfrench2025.pyopus/import/table_schemas/internal_def_product_types.jsonopus/import/table_schemas/obs_files.jsonopus/import/table_schemas/obs_general.jsonopus/import/table_schemas/obs_pds.jsonopus/import/table_schemas/obs_ring_geometry.jsonscripts/server/import_and_deploy/_opus_import_volumes.sh
…e_shelves_only method, ensuring compatibility with PDS4 products.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@opus/import/main_opus_import.py`:
- Around line 419-423: The code unconditionally calls
Pds3File.require_shelves(True) and Pds4File.require_shelves(True), which
violates the --dont-use-shelves-only flag; change the logic to honor
impglobals.ARGUMENTS.dont_use_shelves_only by only calling
Pds3File.require_shelves(True) and Pds4File.require_shelves(True) when
dont_use_shelves_only is False (mirror the existing conditional around
use_shelves_only()), so PDS4 shelf requirement is not forced when the flag is
set and the do_import.py skip logic for missing shelves remains effective.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd5febbe-3168-4fb6-bcd0-e8f2018a11a3
📒 Files selected for processing (1)
opus/import/main_opus_import.py
rfrenchseti
requested changes
Apr 1, 2026
| "version_name": "Current" | ||
| }, | ||
| { | ||
| "category": "Cassini ISS F Ring Reprojected Image", |
Collaborator
There was a problem hiding this comment.
For consistency I think this needs to be "Images".
| def field_obs_wavelength_wave_no_res2(self): | ||
| return self.field_obs_wavelength_wave_no_res1() | ||
|
|
||
| def field_obs_wavelength_polarization_type(self): |
|
|
||
|
|
||
| ############################################## | ||
| ### FIELD METHODS FOR obs_instrument_coiss # |
|
|
||
|
|
||
| ############################################## | ||
| ### FIELD METHODS FOR obs_instrument_coiss # |
Collaborator
There was a problem hiding this comment.
Fix ###
Now that we have two volumes/bundles for COISS, we should have an obs_cassini_iss_common.py, obs_cassini_iss_common_pds3.py, and obs_cassini_iss_common_pds4.py. Both obs_volume_coiss_12xxx.py and this file should use it. The main common file should define all of the fields as None. The PDS3 version should have all of the stuff from coiss_12xxx (and delete it from there). The PDS4 version should try to fill in all of the fields using standard PDS4 class names. See any reprojected image label file for a full set of PDS4 Cassini ISS attributes.
Then remove the default items from this section and leave the ones necessary to override, since we don't actually provide any of these items in the mosaic index file.
| for VOLUME in \ | ||
| EBROCC \ | ||
| uranus_occs_earthbased \ | ||
| cassini_iss_fring_mosaics_rsfrench2025 \ |
… pi_form_type in obs_ring_geometry.json from generic_angle to distance_resolution for projected longitudinal resolution fields.
…Cassini ISS F Ring Reprojected Image" to "Cassini ISS F Ring Reprojected Images" for consistency across multiple entries.
…ernal_def_product_types.json for clarity and consistency. Modify ObsBundleCassiniIssFringMosaicsRSFrench2025 class to return camera-specific shutter mode ID.
…iss_fring_mosaics_rsfrench2025 in _import_all_internal.sh and correcting a comment typo in do_import.py for clarity.
…or clarity, changing 'projected_long_resolution1' and 'projected_long_resolution2' to 'projected_long_resolution_angle1' and 'projected_long_resolution_angle2' respectively.
…nts for clarity and ensuring consistent shelf requirements for both PDS3 and PDS4 files.
… imports for consistency, changing class name to ObsBundleCassiniISSFRingMosaicsRSFrench2025 in relevant files.
…ne for camera-specific shutter mode ID method, enhancing clarity and consistency in instrument-related methods.
…S4 subclasses to streamline method calls, ensuring consistent handling of observation names across different formats.
…nding longitude in ObsBundleCassiniISSFRingMosaicsRSFrench2025 class. Refactor spacecraft clock count methods in ObsCassiniCommonPDS4 for improved error handling and consistency.
…thods for ring geometry calculations, including north-based incidence and emission angles, solar and observer ring opening angles, and a method to determine if the north side of the ring is lit. Refactor existing longitude methods for improved accuracy.
…osaicsRSFrench2025 class to directly use _index_col for minimum and maximum radial resolutions, improving code clarity and reducing redundancy.
…k for None values in the observation ID field, ensuring consistent return values and preventing potential attribute errors.
… _coiss_camera_letter_pds4 method for improved clarity and error management. Update field_obs_instrument_coiss_combined_filter to utilize the new method, ensuring consistent handling of None values and enhancing logging for unexpected shutter mode IDs.
…classes by adding checks for None values in the _is_ring_north_side_lit method. This ensures consistent return values and prevents potential attribute errors when the sunlit side of the rings is unknown.
Collaborator
Author
|
All comments addressed. |
rfrenchseti
requested changes
Apr 21, 2026
| def field_obs_profile_quality_score(self): | ||
| return self._create_mult(self._index_col('rings:data_quality_score')) | ||
|
|
||
| # TODO: Investigate further and fix if necessary. For the moment, assume BeckerJarmak optical |
Collaborator
There was a problem hiding this comment.
All of this BeckerJarmak stuff doesn't make sense here - this is the Uranus occs file
| def field_obs_profile_quality_score(self): | ||
| return self._create_mult(self._index_col('rings:data_quality_score')) | ||
|
|
||
| # TODO: Investigate further and fix if necessary. For the moment, assume BeckerJarmak optical |
Collaborator
There was a problem hiding this comment.
Likewise remove the Uranus stuff from here
| target_desc = coiss_target_desc_mapping[target_desc] | ||
| return self._create_mult(target_desc) | ||
|
|
||
| def _coiss_camera_letter_pds4(self): |
Collaborator
There was a problem hiding this comment.
This is a real hack. Maybe this is only needed for the Mosaics and should be moved to that file? For the real ISS bundles (that we aren't importing yet) we should get the camera from the LID.
| def _camera(self): | ||
| return self._index_col('min_image_name')[-1].upper() | ||
|
|
||
| def _circumference(self): |
Collaborator
There was a problem hiding this comment.
This should be a global constant:
_F_RING_CIRCUMFERENCE = 2 * math.pi * 140221.3
| return self._index_col('rings:maximum_inertial_ring_longitude') | ||
|
|
||
| # Phase angle: The angle between the point where incoming source photons | ||
| # hit the ring , to the direction where outgoing photons to the observer |
Collaborator
There was a problem hiding this comment.
Extra space before ,
Not sure why you're defining phase angle at all here, though.
| self.field_obs_ring_geometry_ascending_longitude2()) | ||
|
|
||
| def field_obs_ring_geometry_ascending_longitude1(self): | ||
| # minimum_longitude_ascending_node |
Collaborator
There was a problem hiding this comment.
This comment and the one below seem redundant with the function name
|
|
||
| # Source: star, observer: COISS | ||
| # Incidence angle: the angle between the point where incoming source photons | ||
| # hit the ring, to the north pole of the planet we're looking at (normal vector |
Collaborator
There was a problem hiding this comment.
The lit side is not the north side! It's the south pole when the lit side is the south side of the rings.
But not sure why any of these comments are here - these are just restating the definitions of these fields.
| ############################################################## | ||
|
|
||
| def field_obs_mission_cassini_obs_name(self): | ||
| # Strip leading/trailing whitespace from the label value |
Collaborator
There was a problem hiding this comment.
We shouldn't need to do this here. It should be taken care of where the table is read in the first place.
…lue checks and enhance clarity. Update filter assignment logic to ensure proper handling of polarization filters and streamline wavelength comparison conditions.
… handling of polarization filters and streamline None value checks. Remove commented-out code for clarity.
…h2025 class for improved clarity and consistency.
… by removing unnecessary None checks and comments for improved clarity and consistency.
… plans for PDS3-specific functionality and improve code documentation.
…armak class by removing unnecessary None checks and comments for improved clarity and consistency. Update handling of optical depth parameters to streamline code.
…ter method of ObsCassiniCommonPDS4 class to improve clarity by rearranging code structure. Ensure proper handling of filter variables after camera check.
…25 class by changing NOTE_MAPPING to _NOTE_MAPPING for consistency. Replace _circumference method with a constant _F_RING_CIRCUMFERENCE to simplify calculations in field_obs_ring_geometry_projected_long_resolution methods.
…s by removing outdated and unnecessary comments related to ring geometry methods for improved clarity and code cleanliness.
…g outdated comments related to ring geometry methods for improved clarity and code cleanliness.
…25 class to enhance code clarity and maintainability.
Collaborator
Author
|
All comments addressed. |
…if_str helper function to streamline whitespace removal from string values. Simplify field_obs_mission_cassini_obs_name method in obs_cassini_common_pds4.py by directly returning the stripped value without additional checks.
rfrenchseti
requested changes
Apr 22, 2026
|
|
||
| return rows, label | ||
|
|
||
| def _strip_if_str(value): |
Collaborator
There was a problem hiding this comment.
So this is another workaround for a problem that shouldn't be here in the first place. The point is that when the index file is read there should not be any strings with trailing space in it. PdsTable does this correctly, but we are only using it for PDS3 index files. The issue is we are using just plain CSV reading for PDS4, but I'm giving you a fixed-width table, and the csv reading isn't stripping the fields. This will eventually be fixed properly when we use PdsTable to read PDS4 index files, but for now the correct fix is to change the reading of the index file.
Collaborator
Author
There was a problem hiding this comment.
.strip() is done in safe_pdstable_read function now
| ret = self._index_col('rings:lowest_detectable_opacity') # Uranus Occs | ||
| if ret is None: | ||
| ret = self._index_col('rings:lowest_detectable_normal_optical_depth') # BeckerJarmak. Note co-uvis-occ-2016-269-sun-i has -999. | ||
| ret = self._index_col('rings:lowest_detectable_normal_optical_depth') |
Collaborator
There was a problem hiding this comment.
So the fix is good but the comment about the -999 has gone away. But I think this is a very interesting comment. In the current OPUS the -999 is showing up in the search results, which is wrong. I presume when the new index file is created it will have -999 marked as a bad value and PdsTable will mask it out, but I'm going to create an issue about it so we don't forget it. But this is an example of paying attention to details in a way that AI will never do for you.
Collaborator
Author
There was a problem hiding this comment.
Comments are put back
…o address handling of -999 values in optical depth methods for improved data integrity.
…nding spaces from string values when parsing floats, improving data integrity. Remove the _strip_if_str helper function for simplification.
…om 'row' to 'rows' to correct iteration logic, enhancing code accuracy.
…and N1481265970, including metadata, images, and associated data files. Enhance data structure for improved accessibility and organization.
…t result identifiers for Cassini ISS observations, enhancing future test coverage.
…ges, enhancing data representation for empty search scenarios.
… a new product type and correcting formatting for improved data structure.
…onse for Cassini ISS observations, enhancing data representation and accessibility.
…ternal_def_product_types.json, enhancing data representation and accessibility in API responses.
…n API response JSON to improve clarity and consistency.
…oduct types in API response JSON to correct inconsistencies and enhance clarity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(Remember to test all browser sizes)
Description of changes:
obs_bundle_cassini_iss_fring_mosaics_rsfrench2025.pyto defineObsBundleCassiniIssFringMosaicsRSFrench2025class for all f-ring bundle fieldsconfig_bundle_info.py)internal_def_product_types.json)obs_files.json,obs_pds.json, andsearch/models.py)co-iss-n1479208692(f ring),co-iss-fring-mosaic-iss_00ari_spkmovper001_prime, andco-iss-fring-mosaic-iss_183ri_spokemov001_primeapi_product_types_empty_search.jsonto include newly added opus product types for f-ring bundle.test__api_meta_range_endpoints_greaterpixelsize_COISS(test_metadata_api.py)test__api_meta_range_endpoints_lesserpixelsize_COISS(test_metadata_api.py)api_help_bundles.htmltest__api_meta_multsto include the f-ring bundles counts under Cassini category (test_metadata_api.py)co-iss-n1479210132(f ring and b ring) andco-iss-n1481265970(b ring)Note:
_infoshelf-*andchecksums-*in Dropbox underPds4FileTest/pds4-holdingsKnown problems:
Summary by CodeRabbit
New Features
Chores