r.surf.fractal: Added seed option and -s flag to module #3480
Conversation
github-actions
bot
added
raster
| # Set up temporary computational region | ||
| cls.use_temp_region() | ||
| # Only 100,000,000 seem to reasonably (not 100%) ensure that all values | ||
| # are generated, so exceeding of ranges actually shows up. |
There was a problem hiding this comment.
What kind of ranges you are referring to? I didn't see any range check. Usually a tiny computational region suffices to save on CI execution time.
There was a problem hiding this comment.
Would you mind helping me with the test suite please, because I haven't previously worked with them and therefore I was struggling with how to write them.
Thanks @marisn
There was a problem hiding this comment.
See for example test for r.random.cells and test documentation. Run r.surf.fractal first with certain fixed seed and save the raster statistics (computed with r.univar) for reference in the test. You can then use assertRasterFitsUnivar to ensure the test-generated raster with the same fixed seed will always give the same values. This type of test is not necessarily testing the output is correct, but it helps detect any regressions in the tool in the future and also that with a fixed seed you always get the same results.
Added the VS code settings by mistake, so removing them in this commit.
neteler
changed the title
raster/r.surf.fractal/main.c
Outdated
| struct Option *seed; | ||
| struct Flag *s_flag; |
There was a problem hiding this comment.
Just wondering, is the s-flag needed given that seed is optional?
Or is it added for consistency with current practice in other modules? Possibly redundand? If seed option is given, seed should be used, no?
And looking at the logic in lines 82 to 95 it seems that setting the s-flag has the same effect as not setting the s-flag...
So, maybe the s-flag can be dropped?
There was a problem hiding this comment.
I looked back at when this was introduced in r.mapcalc by Glynn:
My main objection to automatic seeding is that people will inevitably produce non-repeatable results without even realising it.
That's why user needs to choose -s flag or seed option explicitly. So the focus there was reproducibility. We could argue though that convenience is priority an in that case we could decide to drop the -s flag and not introduce it here.
There was a problem hiding this comment.
I wasn't following the development when the seed option was discussed for r.mapcalc. From an exterior point of view, the general trend of all software I have used yet that uses random numbers is that when you can set the seed, it is usually optional and for users who want know they want it reproducible. I almost always say it as an addition, not a requirement for just basic uses.
So, @Shashankss1205's interpretation is just like mine, and his implementation where not setting the -s flag results in a seed being generated, but with a warning with explanations, is my expected behaviour. Plus it doesn't break compatibility as well, and shows what was done. Maybe an even clearer message that makes the user aware and learn why setting a seed is wanted could be even better. If it's a pattern that we have for handling of -s flags, shouldn't these messages be common across GRASS and not specific to each module?
There was a problem hiding this comment.
Looks like the consensus so far is that -s flag could be dropped. @Shashankss1205 could you please rewrite this without the -s flag?
There was a problem hiding this comment.
+1 for dropping the s flag. Outside of this PR, but this will require making it optional wherever it is used.
There was a problem hiding this comment.
Ok, I'll rewrite and send the code, my main aim was to keep the consistency of the previous code changes and abiding by the same rules. Will be happy to make the necessary changes.
Thank you for all of your's valuable insights.
There was a problem hiding this comment.
Are there any suggestions related to the updated code, I think I have removed all occurences of -s_flag from my code, and also I wanted to seek some guidance on how to work further for getting into GSOC'24 with this organization.
There was a problem hiding this comment.
Great! Please introduce yourself on grass-dev mailing list and let us know which idea you would like to work on so that we can further discuss it. See the tips on the idea page as well. You can also start drafting the application based on the template specified here.
wenzeslaus
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| else { | ||
| /* default as it used to be */ | ||
| seed_value = G_srand48_auto(); | ||
| G_verbose_message(_("Warning set option seed. Generated " |
There was a problem hiding this comment.
Remove the first sentence and the "-s"
| # Set up temporary computational region | ||
| cls.use_temp_region() | ||
| # Only 100,000,000 seem to reasonably (not 100%) ensure that all values | ||
| # are generated, so exceeding of ranges actually shows up. |
There was a problem hiding this comment.
See for example test for r.random.cells and test documentation. Run r.surf.fractal first with certain fixed seed and save the raster statistics (computed with r.univar) for reference in the test. You can then use assertRasterFitsUnivar to ensure the test-generated raster with the same fixed seed will always give the same values. This type of test is not necessarily testing the output is correct, but it helps detect any regressions in the tool in the future and also that with a fixed seed you always get the same results.
|
@Shashankss1205 do you plan to keep working on this? There is not that much missing (a simple test would suffice) and fixing the compilation error should be easy. |
Here is my proposed solution to the issue raised #1044.
I have written the code for adding
seedand-s flagfor reproducibility, along with a unit test for verifying it's functionality