Update lronacpho to use LROC's newer photometric model and parameters file #4519

victoronline · 2021-06-08T01:01:52Z

Description

This update needs to set the default photometric model (LROC_Empirical) to the newer model (2019) and look for the default parameters file in the data/mission/calibration directory. If the old parameter file is provided, the old algorithm(2014) will be used.

Default behavior:
Should use a newer photometric model as default with the newer parameters file (NAC_PHO_LROC_Empirical.0003.pvl).

Optional behavior:
If another pvl is provided with and doesn't specify the algorithm version (NAC_PHO_LROC_Empirical.0001.pvl), then the old photometric model will be used.

If the 2014 version is specified, such as in NAC_PHO_LROC_Empirical.0002.pvl, then the old photometric model will be used.

Related Issue

#4512

Motivation and Context

LROC has created a newer photometric model requiring updated parameters.

How Has This Been Tested?

This has been tested by the science team comparing photometric products using both the old and newer models and validated the results.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Documentation change (update to the documentation; no code change)
[] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have read and agree to abide by the Code of Conduct
I have read the CONTRIBUTING document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
I have added myself to the .zenodo.json document.
I have added any user impacting changes to the CHANGELOG.md document.

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.

I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

… get from calibration file but still backwards compatible if old parameters file exists will use old model

victoronline · 2021-08-02T21:58:28Z

What do we need to do to get this moving forward? Can I work on the missing pieces, like gtest, or do I have to wait for the review to be made?

victoronline · 2021-08-22T07:50:38Z

What do we need to do to get this moving forward? Can I work on the missing pieces, like gtest, or do I have to wait for the review to be made?

isis/src/lro/apps/lronacpho/LROCEmpirical.cpp

isis/src/lro/apps/lronacpho/main.cpp

jessemapel · 2021-08-23T15:04:26Z

isis/src/lro/apps/lronacpho/main.cpp

+  if(algoFile.toLower() == "default" || algoFile.length() == 0){
+    GetCalibrationDirectory("", calDir);
+    algoFile = calDir + "NAC_PHO_LROC_Empirical.????.pvl";
+  }


You can embed this logic in the app XML, see how hical specifies this:

https://github.com/USGS-Astrogeology/ISIS3/blob/dev/isis/src/mro/apps/hical/hical.xml#L1485

Thanks Jesse. Updated with suggested changes.

isis/src/lro/apps/lronacpho/main.cpp

victoronline · 2021-11-01T20:23:49Z

@jessemapel I think I hit all items on your review. Let me know if you need anything else before proceeding.

victoronline · 2021-11-01T20:34:13Z

If there aren't any issues, is there a chance to get this into the next sprint?

victoronline · 2021-11-08T21:36:31Z

@jessemapel Just making sure this doesn't get lost. Please let me know if you still need anything from me.

jessemapel · 2021-11-09T17:34:10Z

This looks good now. I'm not sure how to handle updating the tests on this. For the moment, can you e-mail me or attach a zip of a test image with the validated output?

victoronline · 2021-11-18T00:09:54Z

Jesse, I'll send that to you here shortly. Thanks.

jessemapel · 2022-04-03T23:13:33Z

For testing on this, I'd like to see one test that uses the old parameter file, this likely already exists, and one test that uses the new parameter file. The tests should check that the proper parameters are actually being used and that the report is correct.

victoronline · 2022-04-18T22:38:42Z

@scsides,

This is the error I'm getting when changing the app to be callable. I'll take a closer look later tonight but if something jumps out, please let me know. It's probably an easy typo.

In file included from /ser/estore/scratch/isis/development/git_dev/ISIS3/isis/src/lro/apps/lronacpho/lronacpho.h:6:0, from /ser/estore/scratch/isis/development/git_dev/ISIS3/isis/src/lro/apps/lronacpho/main.cpp:2: inc/Isis.h:9:2: error: #error *****Isis.h MUST be included before any other files!***** #error *****Isis.h MUST be included before any other files!***** ^~~~~ In file included from /ser/estore/scratch/isis/development/git_dev/ISIS3/isis/src/lro/apps/lronacpho/lronacpho.h:6:0, from /ser/estore/scratch/isis/development/git_dev/ISIS3/isis/src/lro/apps/lronacpho/main.cpp:2: inc/Isis.h: In function ‘std::map<QString, void*> GuiHelpers()’: inc/Isis.h:71:27: error: redefinition of ‘std::map<QString, void*> GuiHelpers()’ std::map<QString, void *> GuiHelpers() { ^~~~~~~~~~ In file included from /ser/estore/scratch/isis/development/git_dev/ISIS3/isis/src/lro/apps/lronacpho/main.cpp:1:0: inc/Isis.h:71:27: note: ‘std::map<QString, void*> GuiHelpers()’ previously defined here std::map<QString, void *> GuiHelpers() { ^~~~~~~~~~

Kelvinrr · 2022-04-19T00:19:15Z

@victoronline I can look at the code if need be, the fact that there seems to be a merge issue here (look at files changed) makes it kinda hard. But basically that error is telling you that ISIS.h is not included where it needs to be. It should only be at the very top of the main.cpp where IsisMain is located and not in <app>.cpp where the app-as-funcs are being defined. Common mistake if you copy pasted code over and forgot to delete #include "Isis.h" at the top.

scsides · 2022-04-19T03:17:46Z

@victoronline: In the version I pulled down earlier, lronacpho.cpp was included, but I don't see it now. It had a semicolon at the end of the #include for Isis.h, but Isis.h only needs to be included in the main. Isis.h actually houses the C++ int main(argc, argv){} function and should not be include anywhere else.

scsides · 2022-04-19T03:32:23Z

lronacpho.cpp has a { on line 6. Should be ;

From what I'm seeing, the Isis.h error was masking compile/syntax errors like the one above

victoronline · 2022-04-19T16:21:17Z

I'm fixing the merge errors. I'll upload it again here shortly.

…tory for gtest

jlaura · 2022-04-26T16:54:21Z

@Kelvinrr Are you able to take a look at this today and get a review back to @victoronline?

@victoronline This has merge conflicts that are going to need to be fixed before it is mergeable. Let us know if you have questions about how to handle those.

Thanks!

victoronline · 2022-04-26T20:02:34Z

@jlaura Merge conflicts resolved.

victoronline · 2022-04-26T20:05:01Z

Should I also check the test cub into the data directory?

jlaura · 2022-04-26T21:14:12Z

@victoronline how large is the test cub? We might want to crop not down first if possible.

@Kelvinrr @jessemapel what is the best method to get test data from an external source? Email if small enough and we will check in to the test data on PR merge?

Kelvinrr · 2022-04-26T21:19:19Z

isis/tests/FunctionalTestsLronacpho.cpp

+
+  ASSERT_TRUE(prefix.isValid());
+
+  QString inCubeFileName = "data/lronacpho/M143947267L.cal.echo.crop.cub";


Assuming this image is small enough (like single digit MB), you can just check it in as part of this PR. It seems you already cropped it so I image its good as is.

It is not a single-digit MB. It is 8.4 MB

I'll crop down to 1 MB and add to PR

Added test cube

victoronline · 2022-04-26T22:18:45Z

@jlaura I didn't see your comment before I added the test cube. I can still email if needed.

jlaura · 2022-04-26T22:33:31Z

@victoronline Nope, less than a MB and in the data area like @Kelvinrr said is great!

@Kelvinrr is going to give the updated code a review so that we can get it merged since a number of commits have come in since the original review.

Kelvinrr

Just a few questions, overall looks fine to me. The only other change I would make before approving is adding a line to CHANGELOG.MD under "Unreleased", basically a short description of the change with a link to the relevant issue similar to the history logs. After that I'll approve assuming no other objections from anyone else and we can get this merged.

Kelvinrr · 2022-04-26T23:25:11Z

isis/src/lro/apps/lronacpho/LROCEmpirical.cpp

+        pars.aTerms.push_back(toDouble(ConfKey(profile, "A" + toString(i), toString(0.0))));
+    for (int i=0; i<7; i++)
+        pars.bTerms.push_back(toDouble(ConfKey(profile, "B" + toString(i), toString(0.0))));


I noticed Jesse suggested changing the toString call with the string literal "0.0", and that you made the change, was this reverted somehow? Or do you need to explicitly cast to a QString?

This must have gotten reverted when I corrected the merge issue. Thanks. Fixed in code.

Kelvinrr · 2022-04-26T23:33:37Z

isis/src/lro/apps/lronacpho/LROCEmpirical.cpp

    pars.wavelength = toDouble(ConfKey(profile, "BandBinCenter", toString(Null)));
    pars.tolerance = toDouble(ConfKey(profile, "BandBinCenterTolerance", toString(Null)));
    //  Determine equation units - defaults to Radians
    pars.units = ConfKey(profile, "Units", QString("Radians"));
    pars.phaUnit = (pars.units.toLower() == "degrees") ? 1.0 : rpd_c();
+    pars.algoVersion = toInt(ConfKey(profile, "AlgorithmVersion", QString("0")));


same string question here.

This must have gotten reverted when I corrected the merge issue. Thanks. Fixed in code.

Kelvinrr · 2022-04-26T23:36:39Z

isis/src/lro/apps/lronacpho/lronacpho.h

+  extern void lronacpho(Cube *iCube, UserInterface &ui, Pvl *log=nullptr);
+  extern void lronacpho(UserInterface &ui, Pvl *appLog=nullptr);


kinda minor, but might as well change appLog here to log for consistency with how they're defined in the rest of the lronacpho signatures.

Updated in code.

Kelvinrr · 2022-04-26T23:46:01Z

isis/tests/FunctionalTestsLronacpho.cpp

+    LineManager oLine(outputCube);
+    oLine.SetLine(300);
+    outputCube.read(oLine);
+
+    EXPECT_NEAR(oLine[300],algo3Result, 0.002);
+    outputCube.close();


Anything particular about this DN that makes it important to check or is it a random sample?

This is just a random sample compared to the same output using fx equation the science team is using manually. I would've included testing against fx so any random pixel value could be checked but fx isn't yet callable. This should be updated once it is in case a cube that doesn't have (300,300) pixel.

jlaura · 2022-04-27T19:30:36Z

@victoronline Do you need anything else from us on this right now or are you good with making the minor adjustments based on the re-review? Then we can get this merged and start on the next PR.

victoronline · 2022-04-27T19:37:12Z

@jlaura I should be able to address the items above shortly. Appreciate the note.

…l test

victoronline · 2022-04-27T20:24:34Z

I don't know the etiquette regarding who does the "Resolve conversation" here so I left it up to the reviewer to resolve.

victoronline · 2022-04-28T01:48:59Z

@Kelvinrr @jlaura Do you all need anything else from me to get this merged?

Kelvinrr

@victoronline looks good! 🎉

updated to use new lroc empirical model and parameters and ability to…

961eca5

… get from calibration file but still backwards compatible if old parameters file exists will use old model

jessemapel reviewed Aug 23, 2021

View reviewed changes

jessemapel self-assigned this Feb 11, 2022

jessemapel removed their assignment Apr 18, 2022

victoronline force-pushed the dev-lronacpho-1 branch 2 times, most recently from 0a48527 to 961eca5 Compare April 19, 2022 02:49

victoronline force-pushed the dev-lronacpho-1 branch from 2341902 to 961eca5 Compare April 23, 2022 00:28

victoronline added 2 commits April 22, 2022 17:39

refactored app to be callable and created gtests

914730d

removed unnecessary new lines and added items to lronacpho data direc…

29781cc

…tory for gtest

victoronline force-pushed the dev-lronacpho-1 branch from 17a4b3a to 29781cc Compare April 26, 2022 19:59

Merge branch 'dev' into dev-lronacpho-1

53fb493

Kelvinrr reviewed Apr 26, 2022

View reviewed changes

cropped adding test cub

957f175

Kelvinrr reviewed Apr 26, 2022

View reviewed changes

fixed issues from code review and added history comments to functiona…

b987898

…l test

Kelvinrr approved these changes Apr 28, 2022

View reviewed changes

Kelvinrr merged commit c9f5f8d into DOI-USGS:dev Apr 28, 2022

jessemapel mentioned this pull request Jun 6, 2022

Update to lronacpho to use LROC's newer photometric model and get parameters file from mission calibration directory #4512

Closed


		ASSERT_TRUE(prefix.isValid());

		QString inCubeFileName = "data/lronacpho/M143947267L.cal.echo.crop.cub";

		extern void lronacpho(Cube iCube, UserInterface &ui, Pvl log=nullptr);
		extern void lronacpho(UserInterface &ui, Pvl *appLog=nullptr);

Update lronacpho to use LROC's newer photometric model and parameters file #4519

Update lronacpho to use LROC's newer photometric model and parameters file #4519

Conversation

victoronline commented Jun 8, 2021 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Licensing

victoronline commented Aug 2, 2021

victoronline commented Aug 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victoronline commented Nov 1, 2021

victoronline commented Nov 1, 2021

victoronline commented Nov 8, 2021

jessemapel commented Nov 9, 2021

victoronline commented Nov 18, 2021

jessemapel commented Apr 3, 2022

victoronline commented Apr 18, 2022

Kelvinrr commented Apr 19, 2022

scsides commented Apr 19, 2022 • edited Loading

scsides commented Apr 19, 2022

victoronline commented Apr 19, 2022

jlaura commented Apr 26, 2022 • edited Loading

victoronline commented Apr 26, 2022

victoronline commented Apr 26, 2022

jlaura commented Apr 26, 2022

Kelvinrr Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victoronline commented Apr 26, 2022

jlaura commented Apr 26, 2022

Kelvinrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaura commented Apr 27, 2022

victoronline commented Apr 27, 2022

victoronline commented Apr 27, 2022 • edited Loading

victoronline commented Apr 28, 2022

Kelvinrr left a comment

Choose a reason for hiding this comment

victoronline commented Jun 8, 2021 •

edited

Loading

scsides commented Apr 19, 2022 •

edited

Loading

jlaura commented Apr 26, 2022 •

edited

Loading

Kelvinrr Apr 26, 2022 •

edited

Loading

victoronline commented Apr 27, 2022 •

edited

Loading