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

(v3.3.8) - Observation normalization bug Fix (again), negative values in obs_rms.var #422

Merged
merged 6 commits into from
Jul 3, 2024

Conversation

AlejandroCN7
Copy link
Member

Description

We are continuing to work on PR #420 and #419. The error we thought was resolved actually wasn't; the supposed fix didn't correctly save the calibrations, which prevented the appearance of NaNs.

Here's an explanation of the issue for documentation purposes. If there are negative values in the var values during normalization, the final observations will contain NaNs. When these calibrations are loaded into a model for evaluation, the agents in SB3 return NaNs in all their action variables. This makes debugging difficult, especially when performing intermediate evaluations to save the best model during training.

The issue only occurred in specific buildings and climates because the environment mistakenly saved mean as the var property. This didn't affect the normalization process immediately since it only happened when retrieving the data, so normaliztion in training is working perfectly. However, during evaluation, if the mean had negative values, it caused failures. If there were no negative values, the evaluation process didn't fail outright but was still incorrect.

This PR definitively fixes the issue, marking it as resolved. Additionally, some minor improvements have been made and are documented in the changelog.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)
  • Improvement (of an existing feature)
  • Others

Checklist:

  • I've read the CONTRIBUTION guide (required)
  • My change requires a change to the documentation.
  • I have updated the tests.
  • I have updated the documentation accordingly.
  • I have reformatted the code using autopep8 second level aggressive.
  • I have reformatted the code using isort.
  • I have ensured cd docs && make spelling && make html pass (required if documentation has been updated.)
  • I have ensured pytest tests/ -vv pass. (required).
  • I have ensured pytype -d import-error sinergym/ pass. (required)

Changelog:

  • Evl Callback: Using argument train_env instead of inhereted training environment.
  • Evl Callback: Fixed mean and var normalization calibration set (now it is applied correctly).
  • Normalization Wrapper: Deleted RecordConstructorArgs.
  • Normalization wrapper: Deleted RecordConstructorArgs inherit.
  • Normalize Wrapper: Fixed var property bug (returning mean again instead of var).

@AlejandroCN7 AlejandroCN7 merged commit d7d8c6e into main Jul 3, 2024
4 checks passed
@AlejandroCN7 AlejandroCN7 deleted the fix/bug-normalization branch July 3, 2024 10:36
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.

1 participant