-
Notifications
You must be signed in to change notification settings - Fork 36
Kernels key #675
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
base: main
Are you sure you want to change the base?
Kernels key #675
Conversation
ale/base/data_naif.py
Outdated
| else: | ||
| self._kernels = self._props['kernels'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth checking if it's a dict formatted like a spiceQL kernel set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you envisioning something that checks the kernel keys against valid kernel type names (like [ck, pck, spk, etc.])?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, basically. The easiest thing is probably to try to load it as a kernel set? Might need a function in SpiceQL to check.
ale/drivers/__init__.py
Outdated
| res.instrument_id | ||
| with res as driver: | ||
| isd = formatter(driver) | ||
| if 'remove_kernels' in props and props['remove_kernels'] is True and 'kernels' in isd: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better for it to be "attach_kernels", having a flag being a often a bit odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm, we want kernels added to ISD as default? We can have attach_kernels be default True and users can explicitly set it to False is they want kernels removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, change to attach_kernels and default to true.
ale/formatters/formatter.py
Outdated
| # else: | ||
| # isd["kernels"] = {} | ||
| if 'kernels' in driver_data and isinstance(driver.kernels, dict): | ||
| isd["kernels"] = {k: v for k, v in driver.kernels.items() if not "_quality" in k and not driver.spiceql_mission in k } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep the quality keys. Otherwise how would they know the quality of the CKs/SPKs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the quality omission was in the original logic but I can add them back.
| kernels_dict = {} | ||
| for k in driver.kernels: | ||
| k_split = k.split('/') | ||
| k_type = k_split[2] | ||
| if k_type in kernels_dict and isinstance(kernels_dict[k_type], list): | ||
| kernels_dict[k_type].append(k) | ||
| else: | ||
| kernels_dict.update({k_type: [k]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this is to try to identify the kernel types? Might be best to make that a function in SpiceQL, since SpiceQL can use the Config to identify them more effectively. Short term I think it's fine to leave this here.
| def test_load_kernels(test_kernels): | ||
| label_file = get_image_label('AS15-M-1450', 'isis3') | ||
| compare_dict = get_isd("apollometric") | ||
| isd_str = ale.loads(label_file, props={'kernels': test_kernels}, verbose=True) | ||
| isd_obj = json.loads(isd_str) | ||
| print(json.dumps(isd_obj, indent=2)) | ||
| print("======================") | ||
| print(json.dumps(compare_dict, indent=2)) | ||
| assert isd_obj['kernels']['misc'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With how much new logic you added, you should also test the dict case. Also, if a list is formatted as expected if you are trying to identify kernel types.
Couple things
kernelskey to ISDremove_kernelskey option to for thepropsdict in ale.loads()misckey if user enters their own kernelsLicensing
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: