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.

Sign up for GitHub

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

Jump to bottom

writing test for calibrate with data mapped to observables #450

Merged
merged 12 commits into from
Feb 13, 2024
Merged

writing test for calibrate with data mapped to observables #450

merged 12 commits into from
Feb 13, 2024

Conversation

sabinala
Copy link
Contributor

@sabinala sabinala commented Jan 9, 2024 •
edited
Loading

This PR adds a test to make sure that calibrate works when data is mapped to observables, rather than state variables (in which case the test should be skipped).

Closes #439
(Note that this doesn't solve the issue, just creates a test to address it! See Issue #447 for the fix)

@sabinala sabinala added the WIP PR submitter still making changes, not ready for review label Jan 9, 2024
@sabinala sabinala requested a review from SamWitty January 9, 2024 22:52
@sabinala sabinala self-assigned this Jan 9, 2024
@sabinala sabinala linked an issue Jan 9, 2024 that may be closed by this pull request
Calibrate does not seem to work when data is mapped to observables #439
Closed
@SamWitty
Copy link
Contributor

@sabinala , I see that you requested my review, but the test is marked as WIP and the tests still fail. Is this intended to be reviewed now, or are you still working on it?

@sabinala sabinala removed the request for review from SamWitty January 10, 2024 17:08
@sabinala
Copy link
Contributor Author

sabinala commented Jan 10, 2024 •
edited
Loading

@SamWitty sorry, I should have just messaged you on here instead of requesting your review! My intention was to ask that you take a look at this and let me know what I can do to fix it so that the tests pass.

sabinala and others added 4 commits January 15, 2024 09:26
@sabinala
Update test_interfaces.py
fb1796c 
Changing `@pytest.skip` to `@pytest.skip("Message")`
@sabinala
reformatting
9d29c8a
@sabinala
Update test_interfaces.py
9f871da 
Changing @pytest.skip to @pytest.mark.skip
@SamWitty
Add attribute to ModelFixture
ddef47e
SamWitty
SamWitty reviewed Jan 17, 2024
View reviewed changes
tests/test_interfaces.py Outdated Show resolved Hide resolved
@SamWitty
Copy link
Contributor

@sabinala , with my changes to the linter is this now ready for review?

@sabinala
Copy link
Contributor Author

@SamWitty yes, and thank you for making those changes!

@sabinala sabinala added awaiting review PR submitter awaiting code review from reviewer and removed WIP PR submitter still making changes, not ready for review labels Jan 18, 2024
SamWitty
SamWitty requested changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

@sabinala, looks good, but this can be made simpler and reduce redundant tests. See comments.

tests/test_interfaces.py Outdated Show resolved Hide resolved
@sabinala
Copy link
Contributor Author

@SamWitty I removed the test, but kept everything else.

SamWitty
SamWitty requested changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

Apologies, I just noticed another minor issue that I didn't notice previously.

tests/fixtures.py Outdated Show resolved Hide resolved
@sabinala
Merge branch 'main' into 439-calibrate-does-not-seem-to-work-when-dat…
3b08444 
…a-is-mapped-to-observables
@sabinala
Update fixtures.py
87037f0 
Deleting unstable `DATA_PATH2` and changing to `DATA_PATH`
@sabinala
Copy link
Contributor Author

@SamWitty once this PR (DARPA-ASKEM/simulation-integration#68) is merged, this PR #450 should be good to go

@SamWitty
Copy link
Contributor

@SamWitty once this PR (DARPA-ASKEM/simulation-integration#68) is merged, this PR #450 should be good to go

Turns out that I have permissions to approve PRs on simulation-integration. Who knew!

@sabinala
Copy link
Contributor Author

@SamWitty oh noooooooo! One of these tests isn't passing. It looks like an error from dummy_ensemble_sample but I'm not sure what's happening. What do you think? Does this look like a problem with the model?

@SamWitty
Copy link
Contributor

@SamWitty oh noooooooo! One of these tests isn't passing. It looks like an error from dummy_ensemble_sample but I'm not sure what's happening. What do you think? Does this look like a problem with the model?

There's a lot of randomness in the tests, and sometimes they fail. It's very challenging to test algorithms that are fundamentally random. I just rerun failing tests.

@sabinala sabinala merged commit 0584cc1 into main Feb 13, 2024
5 checks passed
@sabinala sabinala deleted the 439-calibrate-does-not-seem-to-work-when-data-is-mapped-to-observables branch February 13, 2024 21:46
@sabinala sabinala mentioned this pull request Mar 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamWitty SamWitty SamWitty approved these changes

Assignees

@sabinala sabinala

Labels
awaiting review PR submitter awaiting code review from reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Calibrate does not seem to work when data is mapped to observables
2 participants
@sabinala @SamWitty