search

DOI-USGS / ISIS3

Integrated Software for Imagers and Spectrometers v3. ISIS3 is a digital image processing software package to manipulate imagery collected by current and past NASA and International planetary missions.
https://isis.astrogeology.usgs.gov
Other
200 stars 168 forks source link

Update .trn files to be PVL compliant #5566

Closed amystamile-usgs closed 2 days ago

amystamile-usgs commented 3 months ago

Description

Related Issue

https://github.com/planetarypy/pvl/issues/108

How Has This Been Validated?

Types of changes

Checklist:

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.

KrisBecker commented 3 months ago

It feels like these are breaking changes to ISIS. I didn't see this discussion in the referenced issue. I do agree it should be fixed.

Will ISIS still support the old format? In other words, is ISIS going to enforce this format by erroring out if an Auto/Optional/Debug keyword is encountered without an = value? If so, is this a breaking change? If not, why not (it is, after all, non-compliant with the spec)?

Consider a change to the ISIS cube file format this is not backward compatible. Would this be a breaking change?

External developers who are working on new, or have existing, ISIS instruments will be required to modify all their translations files accordingly.

This format has been in use in ISIS since its beginning.

Is there intent to make a code change to enforce this long accepted practice in ISIS?

acpaquette commented 3 months ago

@KrisBecker No code changes are planned and all tests using the updated trn files have passed. This change was sparked by #5484, and is correct that parsing the Auto keyword with no value is not PVL compliant. As tested, the change is backwards compatible.

The change is to ensure that others reading the PVL files get the correct result, and so any stand alone keywords need some kind of value.

W.r.t. requiring external devs to make changes to the .trn files. Since the files are now part of ISIS and do not exist in the ISISDATA area as they had in the past, they are versioned with the software so anyone working with 4.0.0+ versions of ISIS will have working .trn files. But again, with no code change this is somewhat of a moot point. We will encourage external devs to make sure new .trn files are PVL compliant but the ISIS software will not require it of them.

KrisBecker commented 3 months ago

So what is your plan to prevent non-compliant .trn files being submitted into ISIS?

Clearly these files are being used by external s/w systems. If you have no tests to detect these situations, this PR does not fix the OP's bug.

acpaquette commented 3 months ago

After some thought I have four ideas (not all of which are independent of each other):

  1. Fix ISIS to be PVL compliant, removing the ability for empty assignment statements and other non-compliant elements to the ISIS PVL library.

This would be the hardest and most time consuming fix, with potential to break old PVL produced by ISIS. I don't know if any other apps in ISIS produce standalone keywords, I only know of the .trn files. If we discover any other issues with the ISIS PVL parsing I would suspect we would break some old data in some capacity.

  1. Write a PVL sanitizer from scratch to check any PVL that is used in ISIS.

If we want to go down this avenue we might as well write new PVL ingestion code for ISIS, so see 1. Time consuming and large overhead/integration time into ISIS.

  1. Fix as needed if we miss non-compliant PVL in review.

This is a weak fix but allows users to ingest the .trn files as needed. In any of these cases outside users would have to write compliant PVL to add to ISIS. The existing .trn files are already PVL non-compliant and haven't been under any strict scrutiny since they were introduced into ISIS.

  1. Continue work on only having 1 ingestion app for ISIS that completely removes the need for ingestion .trn files

This is something the team started under Stuart, Amy, and I. It was promising but again, a lot of work. This would also remove the need for all of the ingestion .trn files.

We can also work on a mix of these solutions, starting with 3 that is this PR. It provides a more immediate fix and doesn't put us under pressure to bring ISIS PVL reading under compliance immediately. Then begin brain storming on either idea 1 or 2 to bring ISIS into PVL compliance. Or restart work on 4 to reduce the number of .trn files within ISIS.

Most of these solutions will have some impact on outside users producing .trn files. The .trn files aren't typically consumed by outside users. I'm not sure why Jay is reading them with an external PVL reader but to some degree he is right that they should be able to be parsed by another PVL library.

W.r.t. "this PR does not fix the OP's bug" I fail to see how. These .trn files are not produced by ISIS. They are copied and changed by a contributor and added into ISIS for various missions to then be consumed by various Classes in ISIS. The last time was by the Orex team 3 months ago. Before that it was 3 years ago when we moved all of the data into the github repo. I agree it makes it more annoying for external contributors but making the .trn files to my knowledge, is a manual process.

I agree this is a short term fix, and doesn't address the root cause of the problem but it's something we would need to do anyway if we brought ISIS up to compliance with the PVL spec.

I think merging this PR to bring current ISIS .trn files into compliance with the PVL spec and making a new issue to bring general ISIS PVL reading and writing into compliance would be the best way forward.

KrisBecker commented 3 months ago

This might be an easy fix with a small code change. Seems to me that explicitly checking for those keywords in this code section out would permanently fix this problem. I first thought simply commenting out the entire section would do it but I suspect it is part of the parsing algorithm that also returns other non-valued PVL keywords such as END_GROUP and END_OBJECT. Not sure if ISIS has a list of all reserved PVL keywords that are allowed to be valueless, but that might be the place to test for these keywords (which would exclude Auto, Optional and Debug). Nonetheless, it would be a useful exercise to make that change and run your test suite to get an idea of the scope of the impact.

Note that you will also find these keywords (Auto, Optional) in some serial number configurations. We absolutely need this capability to generate and distinguish simulated images from other images for control processes. A key part of this process adds a unique label keyword/value that is optional in the serial number configuration.

This issue seems to be the most serious PVL problem ISIS has at the moment. This because other packages provide ISIS PVL compliancy work-arounds ( e.g., planetary/pvl and maybe GDAL). If you were to isolate this issue and fix it, it is one more step to make ISIS PVL-compliant, but also shows that maybe some problems can be handled in isolation on a case-by-case basis.

For consideration, there are other packages that provide explicit extensions to specs, such as nlohmann::json. JSON specs do not support ordering of JSON keys in an object. However, a separate namespace, nlohmann::ordered_json, provides this functionality and isolates it from the standard.

I hope the impact of this fix is not too prominent in ISIS but this code modification would perhaps give information about the scope the problem in ISIS.

I find it difficult to get fully behind the ideas suggested as they all seem to be a significant amount of work given apparent scope and available resources. I would particularly discourage #4. There is a significant amount of diversity in raw science image products that I fear the app would suffer feature creep and never be finished but indefinitely require modifications/enhancements.

acpaquette commented 3 months ago

W.r.t.: "Note that you will also find these keywords (Auto, Optional) in some serial number configurations. We absolutely need this capability to generate and distinguish simulated images from other images for control processes. A key part of this process adds a unique label keyword/value that is optional in the serial number configuration."

This is still possible as no code changes are being implemented. We will instead prompt contributors who are adding valueless keywords to translation files to instead make them a boolean 0|1 option that you just set to 1. We have no plans to modify current functionality at this time.

I understand the options I provided may not be great but I was asked to provide potential solutions in a previous comment. I agree that many of them are not feasible but doing as you have suggested and making changes to how ISIS reads PVL can be difficult to track and may have unintended side effects. I will look into making a small change to the PVL reader to account for reserved keywords but I won't be spending a ton of time on this.

The process for making .trn files in ISIS has never really been documented and the expectations are allowed to change. We won't be removing the valueless keywords nor will there support be remove by this set of changes. We are bringing existing .trnfiles into PVL compliance and will expect the same of external contributors. Camera model/serial number development will remain the same, we just ask that anyone contributing .trn files to make keywords have values before the PR is merged

acpaquette commented 3 months ago

After looking over the API breaking definition provided by ISIS, the .trn files do not fall under any clear jurisdiction for API breaking vs not API breaking. Because of this, we should err on the side of API breaking and wait for a 9.0 release

amystamile-usgs commented 2 days ago

These changes are in PR https://github.com/DOI-USGS/ISIS3/pull/5573