Skip to content
This repository has been archived by the owner on May 2, 2024. It is now read-only.

use small datasets #88

Closed
wants to merge 10 commits into from
Closed

use small datasets #88

wants to merge 10 commits into from

Conversation

cosunae
Copy link
Contributor

@cosunae cosunae commented Oct 7, 2023

Purpose

Change the default datasets used for testing to a small dataset.

Code changes:

  • Change default dataset to a small domain size.
  • Some products (like radar) still require the entire domain size.

Checklist

Before submitting this PR, please make sure:

  • You have followed the coding standards guidelines established at Code Review Checklist.
  • Docstrings and type hints are added to new and updated routines, as appropriate
  • All relevant documentation has been updated or added (e.g. README)
  • Unit tests are added or updated for non-operator code
  • New operators are properly tested

Additionally, if the PR updates the version of the package

  • The new version is properly set
  • A Tag will be created after the bump

Review

For the review process follow the guidelines at Checklist

Copy link
Contributor

@twicki twicki left a comment

Choose a reason for hiding this comment

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

Definitely a step in the right direction. I think we can simplify the fixtures a bit, see here: #90

@@ -36,7 +36,7 @@ def test_ninjo_k2th(data_dir, fieldextra):
assert_allclose(
fs_ds["POT_VORTIC_AT_THETA"],
observed_at_theta["pot_vortic"],
atol=1e-9,
atol=1e-5,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to decrease the absolute tolerance here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why this was necessary. Especially that 1e-5 is at the order of magnitude of the observable signal in the pot vortic field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be maybe an edge effect due to the differential operators and the reduced computational domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know either, but I think a different location of boundaries can affect the differential operators, (since the stencil applied is different at the boundaries), even if the data is the same in the cropped domain.
I checked again and could increase this threshold again until 1e-8. But still the one below (1e-4) fails if I try to relax it.
However I still think that 1e-4 is still in the "ok" range, provide we do not really know how these operators propagate errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering that the expected values are in the 1e-5 range, it doesn't really make sense to me to put in an absolute tolerance of 1e-5. That would be comparable to setting an atol of 1e5 for the pressure field. The value for the rtol is fine in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realised that fieldextra is probably applying the same stencils on the same data so the argument about the edge effects does not hold.

@cfkanesan cfkanesan mentioned this pull request Dec 11, 2023
cfkanesan added a commit that referenced this pull request Dec 18, 2023
## Purpose

Run tests on a reduced test dataset by default to speed up the test suite.

## Code changes:

- Added the custom pytest marker `data` 
- Remove the `system_defintion` module

Superseeds #88
@cfkanesan cfkanesan closed this Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants