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

Add MTK structure indexing tests #761

Closed
wants to merge 21 commits into from

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Jan 10, 2024

Adds a range of tests that should cover all cases of indexing for all relevant problem types. Will need to wait for merging until various fixes in SymbolicIndexingInterface, as well as updated support for indexing parameters via .ps[] notation.

Copy link
Member

@AayushSabharwal AayushSabharwal left a comment

Choose a reason for hiding this comment

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

Tests don't run because Direct isn't defined. They're very comprehensive, but I think ReactionSystem might need to implement SII for this to work.

@TorkelE
Copy link
Member Author

TorkelE commented Jan 18, 2024

Good catch. Practically this will need the latest MTK update to run (and the accompanying PRs), as that (should) fix a lot of the issues, and also enable the struct.ps[p] notation for get/set index which we test here.

Ideally also #735 to make it easier to test the observables.

@TorkelE
Copy link
Member Author

TorkelE commented Jan 18, 2024

Also, are you sure about ReactionSystem needing to implement SII? Whenever a problem is generated, it always has to be converted to the corresponding system type. Except for the stuff which is broken with MTK models as well, things works with ReactionSystems, so I think it should be fine (unless you know something I don't)

@isaacsas
Copy link
Member

ReactionSystems are AbstractSystems that conform to the MTK interface, shouldn't SII work with a general AbstractSystem?

@isaacsas isaacsas mentioned this pull request Feb 28, 2024
47 tasks
@TorkelE TorkelE changed the base branch from master to Catalyst_version_14 April 6, 2024 21:58
@TorkelE
Copy link
Member Author

TorkelE commented Apr 8, 2024

Closed due to git history mess

@TorkelE TorkelE closed this Apr 8, 2024
@TorkelE TorkelE deleted the add_indexing_tests branch June 8, 2024 18:28
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.

None yet

3 participants