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

219 test all models in amr to see if they can be loaded #243

Closed
wants to merge 31 commits into from
Closed

219 test all models in amr to see if they can be loaded #243

wants to merge 31 commits into from

Conversation

djinnome
Copy link
Contributor

No description provided.

@djinnome djinnome linked an issue Jul 12, 2023 that may be closed by this pull request
test all models in AMR to see if they can be loaded #219
Closed
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@djinnome djinnome marked this pull request as draft July 12, 2023 21:55
djinnome and others added 13 commits July 13, 2023 09:28
@djinnome
added models and code and notebooks to support testing the AMR
a305909
@JosephCottam
Utlity to generate AMR expectations in test utiltiies
6e17287
@JosephCottam
First version that writes a list of expectations and can execute it. …
aa74c9b 
…VERY BAD
@JosephCottam
Merge branch 'main' into 219-test-all-models-in-amr-to-see-if-they-ca…
8d95b4d 
…n-be-loaded
@JosephCottam
Autogenerate tests and test those with code-sharing
c180825
@JosephCottam
Adding ignore so test results don't get accidentally pushed.
9b3c0a1
@JosephCottam
Merge branch 'main' into 219-test-all-models-in-amr-to-see-if-they-ca…
65dcd26 
…n-be-loaded
@JosephCottam
Merge branch 'main' into 219-test-all-models-in-amr-to-see-if-they-ca…
c7d429c 
…n-be-loaded
@JosephCottam
Updated expected return type; restored traceback report (in warnings)…
e059856 
…; Diff reporting vs. reference json
@JosephCottam
Expaded diff reporting
6a058e0
@JosephCottam
Integration test now includes calibrate
aa85938
@JosephCottam
Merge branch 'main' into 219-test-all-models-in-amr-to-see-if-they-ca…
342fd9a 
…n-be-loaded
@JosephCottam
Improve report details
79d5dc3
@JosephCottam JosephCottam self-assigned this Jul 14, 2023
JosephCottam
JosephCottam approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@JosephCottam JosephCottam left a comment

Choose a reason for hiding this comment

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

I took over development...somebody else should review it now.

@JosephCottam
Copy link
Contributor

@djinnome I would appreciate some help diagnosing the current issue on this branch.

It seems that the 'calibrate and sample' might have changed its interface...or that the models need to be updated...or something. I'm not sure where to look to figure out the next step. Any advise?

@djinnome
Copy link
Contributor Author

djinnome commented Aug 8, 2023 •
edited
Loading

It looks like many of these errors were caused by sympytorch. What happens if you run the amr tests with compile_rate_law_p=False

Some errors look like the tests were expecting output to be a dataframe, but got a dict instead

@JosephCottam
Copy link
Contributor

JosephCottam commented Aug 8, 2023 •
edited
Loading

@djinnome Adding compile_rate_law_p=False did not change the number of errors (10 fail).

Though true, it seems something that was returning a dataframe now returns a dictionary, that "something" seems to be solutions_to_observations in interface_utils. (The test itself was already adapted to look for dicts instead of dataframes, see line 117, and that is how I got some tests passing yesterday). Is there a something that is causing it to sometimes return a dataframe and sometimes a dictionary? How would I know which to expect (or possibly, now to tell solutions_to_observations which to expect)?

@JosephCottam
Copy link
Contributor

Add a test that FAILS if things that used to fail now pass, include in the prompt the command line that updates the expected-pass/fail list.

@JosephCottam
Copy link
Contributor

@djinnome I extended this with the "unexpected success" message we talked about.

I haven't been able to figure out how to get calibrate & sample working. Can you take try to fix that call? Currently they all fail with messages I don't know how to interpret.

@djinnome
amr expectations have been updated
5c1f338 
                                           Fails  Prior Fails  Passes  Prior Passes
load_and_calibrate_and_sample_petri_model      3            2      21            10
load_and_sample_petri_model                   15           15      24            12
load_petri_model                               1           13      39            27
@djinnome
Merge branch '219-test-all-models-in-amr-to-see-if-they-can-be-loaded…
78adf62 
…' of https://github.com/ciemss/pyciemss into 219-test-all-models-in-amr-to-see-if-they-can-be-loaded
@djinnome
fixed one error
e5b708c
@djinnome
fixed solutions_to_observations such that it handles dataframes where…
49fcf56 
… ['timepoint_id', 'sample_id'] is a multi-index, or not.
@djinnome djinnome mentioned this pull request Aug 31, 2023
Closed
@djinnome djinnome linked an issue Aug 31, 2023 that may be closed by this pull request
Sanity checks for TA2 models #159
Closed
@djinnome
Copy link
Contributor Author

djinnome commented Jan 4, 2024

@JosephCottam Should we resolve this or just retire it?

@SamWitty
Copy link
Contributor

@JosephCottam and @djinnome , closing this PR as I believe the work here has migrated to the simulation-integration repo. Feel free to reopen if you feel this was closed by mistake.

@SamWitty SamWitty closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JosephCottam JosephCottam JosephCottam approved these changes

Assignees

@JosephCottam JosephCottam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test all models in AMR to see if they can be loaded Sanity checks for TA2 models
3 participants
@djinnome @JosephCottam @SamWitty