-
Notifications
You must be signed in to change notification settings - Fork 118
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
aread8 nodata value in version 5.3.8 #228
Comments
This is a relic of not anticipating contributing area results being negative when originally coded, so I thought -1 would be a convenient no data value. It should be possible to change to another value, and a good value may be MISSINGFLOAT defined at line 80 of https://github.com/dtarb/TauDEM/blob/Develop/src/commonLib.h. Then in aread8 https://github.com/dtarb/TauDEM/blob/Develop/src/aread8.cpp it looks like lines 152 would need changing to I find myself a bit unsure whether the constant MISSINGFLOAT will pass correctly into CreateNewPartition so you might need a code construct similar to that around line 269 where a variable is declared and passed in. If you are able to make these changes and recompile/try it out, it may solve your problem. It would also be good to scrutinize the code to see if there are any places it relies on the -1 assumption and address these. This could be changed/fixed in a future release, but I do not have time for this right now. |
Thank you, I'll fork the repo and give this a try. Do you think forking from v5.3.8 or master would be best? |
I suggest forking from the most recent develop. This will catch more recent fixes and refinements (and I do not think any are breaking or incomplete) |
Will do, thanks! |
…epo author). I made code modifications to only src/area.cpp but wrote comments of possible issues that may occure in createpart.h, I will expand on this here. The previous expectation was athe value -1.0f and in createpartion.h this is of type double. The change swaps this value to MISSINGFLOAT which == MAXFLOAT * -1. This creates the 2's compliment of MAXFLOAT. Because this is of type double it should be recieved without issue but, it appears in certain cases, theis variable is recast as int32_t and more worrisome, int16_t, if this is the case, overflow errors may occur. This may be of no significance as the MISSINGFLOAT is a method of error handling not computation. This code has not been tested and should not be merged until that has occued and QC has taken place. DO NOT MERGE UNTIL AFTER QC.
…epo author). I made code modifications to only src/area.cpp but wrote comments of possible issues that may occure in createpart.h, I will expand on this here. The previous expectation was athe value -1.0f and in createpartion.h this is of type double. The change swaps this value to MISSINGFLOAT which == MAXFLOAT * -1. This creates the 2's compliment of MAXFLOAT. Because this is of type double it should be recieved without issue but, it appears in certain cases, theis variable is recast as int32_t and more worrisome, int16_t, if this is the case, overflow errors may occur. This may be of no significance as the MISSINGFLOAT is a method of error handling not computation. This code has not been tested and should not be merged until that has occued and QC has taken place. DO NOT MERGE UNTIL AFTER QC.
First of all, this is really useful software.
We've been using this for many parameter accumulations following the flow direction grid. Recently, we've had some odd behavior with version 5.3.8. Regions of the weighting grid with values of -1 are returned in the output accumulated grid as areas of nodata. I think this is because -1 is hard coded into aread8 as the nodata value. I'll paste some screen shots and descriptions below. Perhaps this is related to #203 or has been fixed in v5.3.9? I've only had success getting v5.3.8 to compile on linux/HPC.
Accumulated parameter grid with -1 areas shown as nodata:
When I convert -1 to float, -0.01, the issue goes away.
Here is a snippet of what may be the offending code. I don't know c++ so I may be way off here.
It would be great to have the output nodata value not be hard-coded and instead have that value be the same as the nodata value be set as the nodata value of the input weighting grid (if properly defined).
Again, this is really great software and has saved us a ton of time.
Thanks,
Theo
The text was updated successfully, but these errors were encountered: