Skip to content
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

Remove FloatReward #829

Merged
merged 3 commits into from
Jan 17, 2024
Merged

Remove FloatReward #829

merged 3 commits into from
Jan 17, 2024

Conversation

ernestum
Copy link
Collaborator

@ernestum ernestum commented Dec 11, 2023

A quick fix for #794.

After the newest release of SB3 we don't need the FloatReward wrapper any more.

Fixes #794

@ernestum ernestum changed the title Remove FloatReward. Fixes #794 Remove FloatReward Dec 11, 2023
Copy link
Collaborator

@taufeeque9 taufeeque9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

For the failing tests, it seems like the issue is with the incompatibility of Ray with Pydantic 2.5+. According to this issue, Ray 2.9 which will be released end of December should fix the error. For now we can add pydantic<2 in the dependencies as a temporary fix.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d74e903) 95.69% compared to head (b74ad0f) 95.69%.

❗ Current head b74ad0f differs from pull request most recent head a55ff9e. Consider uploading reports for the commit a55ff9e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #829      +/-   ##
==========================================
- Coverage   95.69%   95.69%   -0.01%     
==========================================
  Files         102      102              
  Lines        9650     9642       -8     
==========================================
- Hits         9235     9227       -8     
  Misses        415      415              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ernestum
Copy link
Collaborator Author

I had to remove some outdated test code. Would you re-review @taufeeque9 ?

@taufeeque9
Copy link
Collaborator

The pytorch warning check in test_train_dagger_main was introduced in #332. That PR added the np.array(v) copy line to not cause the pytorch warning. I checked locally that changing np.array(v) -> v in line 465 of imitation/data/types.py doesn't result in pytorch raising the warning. So should we make this change as well? Or will it break anything else?

@ernestum
Copy link
Collaborator Author

Hmm the two changes in #332 make no sense to me. As I understand it, np.array(v) should suppress the warning but the the added test code should ensure that the warning pops up. So why did the test not fail back then?
This seems to be somehow related to the save_to_tensor utility, that I am changing right now in #831.
Maybe @AdamGleave or @qxcv can give us some insight here?

@AdamGleave
Copy link
Member

Hmm the two changes in #332 make no sense to me. As I understand it, np.array(v) should suppress the warning but the the added test code should ensure that the warning pops up. So why did the test not fail back then? This seems to be somehow related to the save_to_tensor utility, that I am changing right now in #831. Maybe @AdamGleave or @qxcv can give us some insight here?

IIUC the test is checking that the warning does not occur:

        assert not (
            warning.category == UserWarning
            and "NumPy array is not writeable" in warning.message.args[0]
        )

so I think we can keep that test code in. Only looked at this quickly, quite possibly missing something obvious, please do LMK if so!

@ernestum ernestum merged commit a8b079c into master Jan 17, 2024
15 checks passed
@ernestum ernestum deleted the remove_float_reward branch January 17, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove FloatReward when next SB3 is released
3 participants