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

Bug fix for conditional moment with topography #149

Merged
merged 11 commits into from
Jul 17, 2023

Conversation

malihass
Copy link
Collaborator

The computation of the second moment when using exogenous data was not working when we need to call the first moment in the batch handler. The batch handler needs to call the first moment with low res and the exogenous data rather than just the low res.

  • Created separate batch handler for conditional moment with exogenous data
  • Added test suite to test for the new classes added

@malihass malihass requested a review from bnb32 May 12, 2023 18:10
@malihass malihass requested a review from bnb32 May 17, 2023 17:16
@malihass malihass force-pushed the malihass/nonSepWindCondMom branch from 9849e8c to 4181518 Compare July 7, 2023 18:46
@malihass
Copy link
Collaborator Author

So after testing further, this PR only slows down the way the batches are assembled for the conditional moment model that use topography, which should be expected. @bnb32 I think we can consider this as ready to be re-reviewed and we can revisit it at later times if needed. The execution time is the same when not using sub filter + topography

Copy link
Collaborator

@bnb32 bnb32 left a comment

Choose a reason for hiding this comment

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

looks good to me. thanks for testing so thoroughly.

@malihass malihass merged commit d9aa690 into main Jul 17, 2023
@malihass malihass deleted the malihass/nonSepWindCondMom branch July 17, 2023 16:24
github-actions bot pushed a commit that referenced this pull request Jul 17, 2023
Bug fix for conditional moment with topography
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.

3 participants