Add stochastic taxi (rainy+fickle)

[foreverska]

Description

Adds rainy transition probabilities and fickle passenger to align environment with paper.

Fixes #161

Type of change

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

Checklist:

• I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[pseudo-rnd-thoughts]

Hey @foreverska, thanks for the PR.

Generally the PR looks good.
Could you change the np.random to self.np_random and revert the environment version increment (new features that don't affect default behaviour shouldn't require version bumps)

and a couple of questions before approving and merging

1. Is this backward compatible with default parameters? If it does currently, could we make it backward compatible.
2. Is the probability of the behaviour reported in step and reset correctly?

[foreverska]

@pseudo-rnd-thoughts

Addressed Comments.

Is this backward compatible with default parameters

Yes, default values for rainy and fickle are both False. This aligns functionality with pre-commit default behavior.

Is the probability of the behaviour reported in step and reset correctly?

It matches the other ToyText environments (Cliff/Frozen) with step returning the probability of the taken transition and reset always returning 1. I added lines in the unit test to guard against regression.

[pseudo-rnd-thoughts]

With the tests and documentation updates, then should be good to merge.
Thanks for making the changes

[Kallinteris-Andreas]

Looks good to me,

[foreverska]

@pseudo-rnd-thoughts I think this needs one more review since there was a commit after your last one. Thanks.

[pseudo-rnd-thoughts]

Hey @foreverska, sorry I'm on holiday currently. Looking over the PR again, I'm a tad worried about the is_rainy change.
Is there a way of making the code only run if is_rainy is true?

[foreverska]

Hey @foreverska, sorry I'm on holiday currently. Looking over the PR again, I'm a tad worried about the is_rainy change. Is there a way of making the code only run if is_rainy is true?

Please enjoy vacation and ignore this until you're good and rested.

Is the ask to restore the original code and switch to it when it's not rainy and switch to this new code if it is? Or is there a more pointed change you'd like to see?

[pseudo-rnd-thoughts]

hi @foreverska, I'm back. Looking over the PR with a week or two rest, I agree that I would prefer if the new code was "disabled" by default and wouldn't run at some, unlike the current solution.
I know that this produces arguably less elegant code, I think it will be better maintenance and for people understanding the code.
Could you make that change?

[foreverska]

@pseudo-rnd-thoughts Not a problem at all. Pushed up a change that restores the old code when dry. Let me know what you think.

[pseudo-rnd-thoughts]

To minimise changes, could this function, take row, col, pass_idx, dest_idx, action as arguments that we run the original code unless is_rainy then we call this function.
Then return the data.
It just means that we don't need to copy and paste the massive for loop and is clear what the differences between the current functions is

[foreverska]

I believe I have done a reasonable job at making the code as reusable as possible. Please let me know if you had something else in mind.

[pseudo-rnd-thoughts]

Looks good @foreverska.

One last request is to change the function names to _{name} rather than double underscore to make the style of the rest of the project.
Then we should be good to merge

[foreverska]

Roger, adjusted the function names, ready for final review.