search

ninoxconsulting / PEMprepr

This sub package prepares data for Predictive Ecosystem Mapping workflow
Apache License 2.0
0 stars 0 forks source link

Fix saga #23

Closed gcperk closed 1 day ago

gcperk commented 2 weeks ago

This branch was largely an update of saga scripts (landscape and model covars) to streamline and remove the unwanted sysdata.rda and external data files. I added two internal check function to connect to saga (find_saga_path() and check_saga(). These are currently internal, however they might need to be fully documented as the reference within the create_covariates link is showing up as a Warning?

<body>
<!--StartFragment-->
❯ checking Rd cross-references ... WARNING   Missing link or links in documentation object 'create_covariates.Rd':     'find_saga_path'      Missing link or links in documentation object 'create_samplr_covariates.Rd':     'find_saga_path'      See section 'Cross-references' in the 'Writing R Extensions' manual.  0 errors ✔ \| 1 warning ✖ \| 0 notes ✔
--
 
> | >
>

<!--EndFragment-->
</body>
</html>

This pull request will close #13 .

I added basic test functionality, but interested on your assessment to add a snapshot to test running the entire suite of outputs. I also suspect the internal point to a local .saga exe is causing the tests to fail, so commenting out for now. Is there a nice way to deal with this? ie avoid: sp <- "C:/Programs/saga-9.2.0_x64/saga-9.2.0_x64/saga_cmd.exe" within the testing.

Happy to get your views on overall functionality and any improvements .

gcperk commented 1 week ago

@ateucher - I think this is ready for review, before I get too far down the track. It is throwing an apple/mac error still but everything else is passing. Happy to chat first if that is easier. Thanks!

ateucher commented 1 week ago

Thanks @gcperk! I've started figuring out how to install on a Mac - notes here: https://github.com/ninoxconsulting/PEMprepr/issues/10#issuecomment-2455955103. I think I'm almost there and can then test this :)

Also in that comment is a link to install it on Linux, so we could do that in GitHub actions and test conditionally there...

gcperk commented 1 week ago

thanks @ateucher. I'm not totally clear how much of the setup is required to get the tests to successfully run? Do you need anything from me at the moment? I have a linux box (ubuntu) so I can run through and test locally also.

gcperk commented 1 week ago

Great thanks @ateucher, we completely agree they are long!! I will wait til you are finished with the entire review before I make any changes. A major challenge I have is with the testing as it relies on the saga_path so could see that being an issue. Any ideas there? Thanks for the review as always.

gcperk commented 1 day ago

Thanks @ateucher for the review. I updated the create_covariates to update default dem to sinksfilled and updates the csv alongside. I incorporated the minor fixes you found (thanks!). I will now merge back to main so that leaves the saga backend fix which you have on a separate branch.

ateucher commented 1 day ago

That's perfect, thanks @gcperk - looks good!