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

photomet MinnaertEmpirical will not work with certain formats of pvl input #3621

Closed
krlberry opened this issue Dec 31, 2019 · 6 comments · Fixed by #5175
Closed

photomet MinnaertEmpirical will not work with certain formats of pvl input #3621

krlberry opened this issue Dec 31, 2019 · 6 comments · Fixed by #5175
Assignees
Labels
bug Something isn't working
Milestone

Comments

@krlberry
Copy link
Contributor

krlberry commented Dec 31, 2019

ISIS version(s) affected: all ISIS3 versions

Description
photomet will fail when used with the frompvl parameter and model=MinneartEmpirical if the pvl specified was of the format has keywords of the form:
PvlKeyword = (1,2,3,3,4) rather than PvlKeyword="1,2,3,4"

This is technically documented at: https://isis.astrogeology.usgs.gov/Application/presentation/Tabbed/photomet/photomet.html
but it is easy to generate output of the unsupported format using photemplate, and the unsupported format is the more standard format for multi-value keywords in ISIS, so it should be supported.

How to reproduce
Run photomet using model='MinneartEmpiricall and provide a pvl file with any of the PvlKeywords: PhaseList, KList, or PhaseCurveListset to a series of values formatted as:
PvlKeyword = (1,2,3,3,4)

Possible Solution
Same solution as #3614

Additional context
Noticed that the failure mode reported in #3608 affects MinneartEmpirical as well after the sprint's completion.

@krlberry krlberry added the Missions Issues which are a priority for missions label Dec 31, 2019
@rbeyer
Copy link
Contributor

rbeyer commented Feb 24, 2020

FYI, since I've lately been deep in reading the 'standards' for PVL, it is important to note that this construction: PvlKeyword="1,2,3,4", is a PVL Quoted String assignment statement whereas this construction: PvlKeyword=(1,2,3,4), is a PVL Sequence assignment statement. So in the first case, a standards-compliant PVL parser would assign the value 1,2,3,4 to a string variable, whereas the second would assign the four values to a list or array of the appropriate numerical type, depending on the programming language and the parser.

Both are 'valid' PVL, I suppose, but if this keyword is always going to be a list of numbers, where the order is important, and access to the individual values is relevant, then it really should be constructed as a PVL Sequence, so that PVL parsers can parse and validate the values correctly. Otherwise, you would need some other code to convert the string to a list of numbers and validate that it went correctly, which seems like double the work.

However, I guess that 'double work' is already being done if this program works with the PVL Quoted String construction and not the PVL Sequence construction. It is odd that it was implemented that way.

@jlaura jlaura added bug Something isn't working and removed Missions Issues which are a priority for missions labels Mar 12, 2020
@ascbot
Copy link
Contributor

ascbot commented Sep 1, 2020

I am a bot that cleans up old issues that do not have activity.

This issue has not received feedback in the last six months. I am going to add the inactive label to
this issue. If this is still a pertinent issue, please add a comment or add an emoji to an existing comment.

I will post again in five months with another reminder and will close this issue on it's birthday unless it has
some activity.

@ascbot ascbot added the inactive Issue that has been inactive for at least 6 months label Sep 1, 2020
@krlberry
Copy link
Contributor Author

krlberry commented Sep 2, 2020

This is still an issue.

@krlberry krlberry removed the inactive Issue that has been inactive for at least 6 months label Sep 2, 2020
@github-actions
Copy link

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

@github-actions github-actions bot added the inactive Issue that has been inactive for at least 6 months label May 26, 2021
@jessemapel jessemapel added this to the 5.0.1 milestone May 28, 2021
@jessemapel jessemapel removed the inactive Issue that has been inactive for at least 6 months label May 28, 2021
@github-actions
Copy link

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

@github-actions github-actions bot added the inactive Issue that has been inactive for at least 6 months label Nov 25, 2021
@rbeyer
Copy link
Contributor

rbeyer commented Nov 25, 2021

Still relevant.

AustinSanders pushed a commit that referenced this issue May 22, 2023
…emplate format. Fixes #3621. (#5175)

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format

* Updated CHANGELOG
@github-project-automation github-project-automation bot moved this from In Progress to Done in FY22 Q3 Software Support May 22, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in ASC Software Support May 22, 2023
chkim-usgs added a commit that referenced this issue Jun 22, 2023
* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format. Fixes #3621. (#5175)

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format

* Updated CHANGELOG

* changed description for tgocassisrdrgen to be more descriptive than before (#5167)

* changed description for tgocassisrdrgen to be more descriptive than before.

* modifying the changelog to reflect doc changes

* fix typo

* Documentation fix for broken links in User Docs (#5182)

* doc changes for new links, need to scan other files for the same

* fixed remaining links

* typo

* fixed command line references link for ISIS3 Applications

* Update to Ale 0.9.0 (#5209)

* Update to Ale 0.9.0

* Updated to ALE 0.9.1

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* Updated meta.yaml to reflect 8.0.0 release (#5203)

* Updated meta.yaml to reflect 8.0.0 release

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* Test autolabeler

---------

Co-authored-by: Anton Hibl <[email protected]>
Co-authored-by: Amy Stamile <[email protected]>
Co-authored-by: acpaquette <[email protected]>
Co-authored-by: Austin Sanders <[email protected]>
chkim-usgs added a commit that referenced this issue Jun 23, 2023
* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format. Fixes #3621. (#5175)

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format

* Updated CHANGELOG

* changed description for tgocassisrdrgen to be more descriptive than before (#5167)

* changed description for tgocassisrdrgen to be more descriptive than before.

* modifying the changelog to reflect doc changes

* fix typo

* Documentation fix for broken links in User Docs (#5182)

* doc changes for new links, need to scan other files for the same

* fixed remaining links

* typo

* fixed command line references link for ISIS3 Applications

* Update to Ale 0.9.0 (#5209)

* Update to Ale 0.9.0

* Updated to ALE 0.9.1

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* Updated meta.yaml to reflect 8.0.0 release (#5203)

* Updated meta.yaml to reflect 8.0.0 release

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* Test autolabeler

* Try linking og issue

---------

Co-authored-by: Anton Hibl <[email protected]>
Co-authored-by: Amy Stamile <[email protected]>
Co-authored-by: acpaquette <[email protected]>
Co-authored-by: Austin Sanders <[email protected]>
@AustinSanders AustinSanders mentioned this issue Dec 6, 2023
12 tasks
AustinSanders added a commit that referenced this issue Dec 6, 2023
* Cnetthinner application bug fix resolving divide by zero in CnetManager.cpp. (#5356)

* Bug fix for a divide by zero in CnetManager.cpp. Additionally, the cnetthinner app has been converted to a callable function and Makefile tests converted to gtests. Addresses #5354.

* Updated CHANGELOG.md. Addresses #5354.

* Minor change to cnetthinner.xml. Addresses #5354.

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format. Fixes #3621. (#5175)

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format

* Updated CHANGELOG

* Dawn Target Translation Fix (#5294)

* Fixes dawn target translation for CERES images

* Added Changelog entry

* CSM Camera Qview Try Catches (#5295)

* Fix errors when using CSM camera in qview

* Added Changelog entry

* Fixes CSV parsing in shadowtau (#5316)

* Fixes CSV parsing in shadowtau

* Addressed PR feedback

* Error Report Fix for Program Launcher (#5331)

* Removed line that gets class, it is derived from the code

* Added more detailed error around itime not finding the leapsecond kernel

* Added changelog entry

* Updated trimfilter to correctly use high/low filters (#5340)

* Added high/low filters to trimfilter

* Updated changelog

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* corrected calibration inclusion filter (needed ,) (#5357)

* corrected calibration inclusion filter (needed ,)

* changelog entry downloadIsisData fix

* Updated changelog

* Updated changelog

* Cnetthinner application bug fix resolving divide by zero in CnetManager.cpp. (#5356)

* Bug fix for a divide by zero in CnetManager.cpp. Additionally, the cnetthinner app has been converted to a callable function and Makefile tests converted to gtests. Addresses #5354.

* Updated CHANGELOG.md. Addresses #5354.

* Minor change to cnetthinner.xml. Addresses #5354.

* The cnetedit application has been refactored to be callable; Makefile tests have been removed and replaced by gtests (#5348)

* The cnetedit application has been refactored to be callable; Makefile tests have been migrated to gtest format. Addresses #5346.

* Removed extraneous variables from FunctionalTestsCnetedit.cpp. Addresses #5346.

* Updated gtests; replace cubes with labels. Addresses #5346.

* Per review cleaned up sloppy spacing; made global variables static in cnetedit.cpp to avoid issues when linking libisis. Addresses #5346.

* The cnetdiff application has been refactored to be callable; Makefile tests have been removed and replaced by gtests. (#5323)

* The cnetedit applciation has been refactored to be callable; Makefile tests have been removed and replace by gtests. Addresses #5322.

* Moved CHANGELOG.md entry for cnetdiff changes from "Added" section to "Changed" section per reviewer suggestion. Addresses #5322.

* Reverted back to original ControlNetDiff source and header files per reviewer suggestion. Addresses #5322.

* Changes to cnetdiff.cpp and main.cpp to use the addLogGroup method to add PvlGroups to Pvl. Per reviewer suggestion. Addresses #5322.

* Put back line erroneously removed in CHANGELOG.md, per review. Addresses #5322.

* Version ticks

* Updated changelog

* Updated changelog

---------

Co-authored-by: kledmundson <[email protected]>
Co-authored-by: Christine Kim <[email protected]>
Co-authored-by: acpaquette <[email protected]>
Co-authored-by: Jacob Cain <[email protected]>
acpaquette added a commit to acpaquette/ISIS3 that referenced this issue Mar 29, 2024
* Cnetthinner application bug fix resolving divide by zero in CnetManager.cpp. (DOI-USGS#5356)

* Bug fix for a divide by zero in CnetManager.cpp. Additionally, the cnetthinner app has been converted to a callable function and Makefile tests converted to gtests. Addresses DOI-USGS#5354.

* Updated CHANGELOG.md. Addresses DOI-USGS#5354.

* Minor change to cnetthinner.xml. Addresses DOI-USGS#5354.

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format. Fixes DOI-USGS#3621. (DOI-USGS#5175)

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format

* Updated CHANGELOG

* Dawn Target Translation Fix (DOI-USGS#5294)

* Fixes dawn target translation for CERES images

* Added Changelog entry

* CSM Camera Qview Try Catches (DOI-USGS#5295)

* Fix errors when using CSM camera in qview

* Added Changelog entry

* Fixes CSV parsing in shadowtau (DOI-USGS#5316)

* Fixes CSV parsing in shadowtau

* Addressed PR feedback

* Error Report Fix for Program Launcher (DOI-USGS#5331)

* Removed line that gets class, it is derived from the code

* Added more detailed error around itime not finding the leapsecond kernel

* Added changelog entry

* Updated trimfilter to correctly use high/low filters (DOI-USGS#5340)

* Added high/low filters to trimfilter

* Updated changelog

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* corrected calibration inclusion filter (needed ,) (DOI-USGS#5357)

* corrected calibration inclusion filter (needed ,)

* changelog entry downloadIsisData fix

* Updated changelog

* Updated changelog

* Cnetthinner application bug fix resolving divide by zero in CnetManager.cpp. (DOI-USGS#5356)

* Bug fix for a divide by zero in CnetManager.cpp. Additionally, the cnetthinner app has been converted to a callable function and Makefile tests converted to gtests. Addresses DOI-USGS#5354.

* Updated CHANGELOG.md. Addresses DOI-USGS#5354.

* Minor change to cnetthinner.xml. Addresses DOI-USGS#5354.

* The cnetedit application has been refactored to be callable; Makefile tests have been removed and replaced by gtests (DOI-USGS#5348)

* The cnetedit application has been refactored to be callable; Makefile tests have been migrated to gtest format. Addresses DOI-USGS#5346.

* Removed extraneous variables from FunctionalTestsCnetedit.cpp. Addresses DOI-USGS#5346.

* Updated gtests; replace cubes with labels. Addresses DOI-USGS#5346.

* Per review cleaned up sloppy spacing; made global variables static in cnetedit.cpp to avoid issues when linking libisis. Addresses DOI-USGS#5346.

* The cnetdiff application has been refactored to be callable; Makefile tests have been removed and replaced by gtests. (DOI-USGS#5323)

* The cnetedit applciation has been refactored to be callable; Makefile tests have been removed and replace by gtests. Addresses DOI-USGS#5322.

* Moved CHANGELOG.md entry for cnetdiff changes from "Added" section to "Changed" section per reviewer suggestion. Addresses DOI-USGS#5322.

* Reverted back to original ControlNetDiff source and header files per reviewer suggestion. Addresses DOI-USGS#5322.

* Changes to cnetdiff.cpp and main.cpp to use the addLogGroup method to add PvlGroups to Pvl. Per reviewer suggestion. Addresses DOI-USGS#5322.

* Put back line erroneously removed in CHANGELOG.md, per review. Addresses DOI-USGS#5322.

* Version ticks

* Updated changelog

* Updated changelog

---------

Co-authored-by: kledmundson <[email protected]>
Co-authored-by: Christine Kim <[email protected]>
Co-authored-by: acpaquette <[email protected]>
Co-authored-by: Jacob Cain <[email protected]>
acpaquette added a commit that referenced this issue Mar 29, 2024
* Cnetthinner application bug fix resolving divide by zero in CnetManager.cpp. (#5356)

* Bug fix for a divide by zero in CnetManager.cpp. Additionally, the cnetthinner app has been converted to a callable function and Makefile tests converted to gtests. Addresses #5354.

* Updated CHANGELOG.md. Addresses #5354.

* Minor change to cnetthinner.xml. Addresses #5354.

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format. Fixes #3621. (#5175)

* Update photomet's MinnaertEmpirical model to handle PVL input in photemplate format

* Updated CHANGELOG

* Dawn Target Translation Fix (#5294)

* Fixes dawn target translation for CERES images

* Added Changelog entry

* CSM Camera Qview Try Catches (#5295)

* Fix errors when using CSM camera in qview

* Added Changelog entry

* Fixes CSV parsing in shadowtau (#5316)

* Fixes CSV parsing in shadowtau

* Addressed PR feedback

* Error Report Fix for Program Launcher (#5331)

* Removed line that gets class, it is derived from the code

* Added more detailed error around itime not finding the leapsecond kernel

* Added changelog entry

* Updated trimfilter to correctly use high/low filters (#5340)

* Added high/low filters to trimfilter

* Updated changelog

* Updated changelog

---------

Co-authored-by: acpaquette <[email protected]>

* corrected calibration inclusion filter (needed ,) (#5357)

* corrected calibration inclusion filter (needed ,)

* changelog entry downloadIsisData fix

* Updated changelog

* Updated changelog

* Cnetthinner application bug fix resolving divide by zero in CnetManager.cpp. (#5356)

* Bug fix for a divide by zero in CnetManager.cpp. Additionally, the cnetthinner app has been converted to a callable function and Makefile tests converted to gtests. Addresses #5354.

* Updated CHANGELOG.md. Addresses #5354.

* Minor change to cnetthinner.xml. Addresses #5354.

* The cnetedit application has been refactored to be callable; Makefile tests have been removed and replaced by gtests (#5348)

* The cnetedit application has been refactored to be callable; Makefile tests have been migrated to gtest format. Addresses #5346.

* Removed extraneous variables from FunctionalTestsCnetedit.cpp. Addresses #5346.

* Updated gtests; replace cubes with labels. Addresses #5346.

* Per review cleaned up sloppy spacing; made global variables static in cnetedit.cpp to avoid issues when linking libisis. Addresses #5346.

* The cnetdiff application has been refactored to be callable; Makefile tests have been removed and replaced by gtests. (#5323)

* The cnetedit applciation has been refactored to be callable; Makefile tests have been removed and replace by gtests. Addresses #5322.

* Moved CHANGELOG.md entry for cnetdiff changes from "Added" section to "Changed" section per reviewer suggestion. Addresses #5322.

* Reverted back to original ControlNetDiff source and header files per reviewer suggestion. Addresses #5322.

* Changes to cnetdiff.cpp and main.cpp to use the addLogGroup method to add PvlGroups to Pvl. Per reviewer suggestion. Addresses #5322.

* Put back line erroneously removed in CHANGELOG.md, per review. Addresses #5322.

* Version ticks

* Updated changelog

* Updated changelog

---------

Co-authored-by: kledmundson <[email protected]>
Co-authored-by: Christine Kim <[email protected]>
Co-authored-by: acpaquette <[email protected]>
Co-authored-by: Jacob Cain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
7 participants