WISE-Developers / Project_issues

This handles incoming tickets like bugs and feature requests
GNU Affero General Public License v3.0
2 stars 0 forks source link

[WISE Task]: FBPTester update #69

Open RobBryce opened 2 years ago

RobBryce commented 2 years ago

Contact Details

rbryce@heartlandsoftware.ca

What is needed?

The FBPTester program runs standard tests against CFS provided code. It may be appropriate to obtain any new code update from CFS (we haven't done this in a decade) to determine if any updates that may have been made, impact our unit tests.

How will this improve the project or tool?

If we are validating against an old code base that needs updated, we may have some code that isn't correct/current.

BadgerOnABike commented 2 years ago

This is an interesting concept as the active dev code for CFFDRS is now in R. Perhaps we can leverage that though.

spydmobile commented 2 years ago

This is an interesting concept as the active dev code for CFFDRS is now in R. Perhaps we can leverage that though.

True, @RobBryce what do you think about using the R lib instead?

RobBryce commented 2 years ago

We could try to consume it. Right now, FBPTester is all in C/C++, so we would have to consider working within the program, or automating test runs from two source trees and comparing results with an external tool. Brett, the filename is "fbpfunc4.6.c", for your reference. Also note that I've been told that there's been a change on opinion at CFS on how to calculate secondary outputs - using equilibrium values, or values at time t.

spydmobile commented 2 years ago

@RobBryce and @BadgerOnABike sounds like there needs to be more conversation before we can action this please confirm?

RobBryce commented 2 years ago

We can act on this. Is the old C code officially dead, or was it kept in sync during the development of the R code? Either way, we for due diligence, we should make sure testing is against current CFS code.

BadgerOnABike commented 2 years ago

Grabbing some extra info based on Robs questions.

spydmobile commented 2 years ago

Waiting on @BadgerOnABike to direct rob on which codebase C++ or R

BadgerOnABike commented 2 years ago

Response I've gotten back is that R is the most up to date. I'll bug Wotton next week also.

BadgerOnABike commented 2 years ago

Wotton said his most up to date was from 2015. Awaiting a code base for another project that we could likely use here.

spydmobile commented 2 years ago

@rob must test and @BadgerOnABike is working on delivering something. Latest 2015 c code of FBP, currently using the 2015 code in FBPT.

spydmobile commented 1 year ago

@RobBryce discuss with @BadgerOnABike and move this along please.

RobBryce commented 1 year ago

@BadgerOnABike, any updates on the code base? Or should we work with the R code you mentioned? Or update the C code to match your R code?

BadgerOnABike commented 1 year ago

Asking again as to if there are any really fundamental changes to the underlying math. I know a number of processes have changed but I'm not sure the math has. That's been the delays here as the folks with knowledge of both code bases have been on roving vacation.

BadgerOnABike commented 1 year ago

Nothing new, we are good to go as is.

RobBryce commented 1 year ago

@BadgerOnABike - can we assume that the R code and C code produce the same outputs, or should we work from the R code?

spydmobile commented 1 year ago

From Exec: @RobBryce this is no longer the direction we wish to go, we dont want to invest in stand alone tools that will be replaced by github build automation, we feel effort will be better spent in doing unit tests in github.

RobBryce commented 1 year ago

@spydmobile - we still need something to run unit tests against. Prior work (years ago) found that the published test data set was incomplete in finding differences between this code base and the CFS.

BadgerOnABike commented 1 year ago

So, you need data to test against. Which we can host somewhere (or calculate on the fly for comparison) that can be picked up by a sub call to the WISE stack that only initializes the FWI/FBP calculation. We can also expand on any limitations found without much issue. Is that an acceptable way to perform these tests? As a stand-alone FBP tester doesn't really make sense when we can just send commands to the components we need to test?

RobBryce commented 1 year ago

@BadgerOnABike - you cannot simply send commands on a CLI to the FBP DLL, either on Linux or Windows. You need some program that sits between the user and the DLL, which receives direction from the command line or GUI. Some confusion may be where this code lives - it's well below any executable or REST API or Builder.

FBPTester, as it exists now, can already be scripted with a set of inputs because it can be run from a command line without any use of the user interface. And we did automate that for a wide range of inputs. FBPTester compared results of this code base against the CFS code base, and never against a predefined set of outputs. It is the tool we already developed to interact with the API to the FBP DLL/module/codebase.

Then, additionally, we automated unit tests against the closed set of inputs defined in the FBP standards documents, and this test set can be expanded upon rather easily. These unit tests, however, automated the execution of FBPTester to (again) compare outputs of 2 programs, not compare outputs against a pre-generated set of expected values. This code was set up to compare the V6 and V7 code bases, both for our code, and the V7 compilation of the CFS code.

Testing the Java code base is another issue outside the scope of this ticket. This code base is a language translation from the WISE/Prometheus code base and is used in REDApp.

WISE could be used to automate one portion of the FBP Tester suite that already exists, and that's comparison of generated polygons against theoretical ellipses, but that's different from the discussion here to simply compare ROS, FI, CFB, etc. FBP statistics against a given input. WISE.EXE (or Prometheus.EXE) does not (itself) expose this type of computation, which is why FBP Tester existed to begin with.

spydmobile commented 1 year ago

@RobBryce , This issue seems to assume there is an updated CFFDRS that you dont have. Thats not true, @BadgerOnABike has confirmed, nothing new, so is there actually anything to this issue? I think not. I think your FBP Tester is fine as is, and likely does not need any work, or effort put into it, so that satisfies, the executive ask above. what EXACTLY do you want, to move forward with this. Please be concise but comprehensive. I think this is a dead issue, but if not, tell me what you are looking for.

RobBryce commented 1 year ago

@spydmobile - you're right, there may be nothing here to do, but we needed to chat through this to make sure there wasn't. @BadgerOnABike did mention that the C code has been replaced with R code. If he's confident that they are functionally equivalent, then we're done. If the R code is the maintained truth source and if it is in any way different from outputs of the last C source code we used, then that should be investigated, though.

spydmobile commented 1 year ago

Ok, so if and when there are significant changes to the R Code, we will need to revisit this ticket. Until then, i want to remind you that way back in redmine 1110 I pushed for an exposure of the FWI/FBP via the javascript API - For different reasons (I wanted to be able to calculate some FWI/FBP without having to run a whole model), sadly it was sent to Neal for approval and died in there, and not intentionally I think. So then I implemented the Mesowest CFFDRS (JS) Library (We talked about this) and we are now also implementing it in The New Wildfire Framwework API

nealmcloughlin commented 1 year ago

I have supported exposing FWI, FBP, and other useful functions like spatial weather modelling from the beginning. I apologize if I failed to approve this work back in redmine. It likely fell through the cracks as everyone on the team assigned their tickets to me for approval and I was overwhelmed.

Water under the bridge at this point. This is still important work and I support it. Exposing the FWI and FBP could be our first step towards automated unit testing.

spydmobile commented 1 year ago

I apologize if I failed to approve this work back in redmine. It likely fell through the cracks as everyone on the team assigned their tickets to me for approval and I was overwhelmed.

@nealmcloughlin Yup, totally get that, no worries there! So I will move this ticket into hibernation and create one each for FWI/FBP and Spatial Wx Modelling.

spydmobile commented 1 year ago

Hey @BadgerOnABike lets decide how we are going to move forward on this, maybe we should consider your idea to make it a wise resource and make it open?

spydmobile commented 1 year ago

sorry all, testing github discord integration