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

drop tests for indexing problem with a parameter #387

Closed
wants to merge 5 commits into from

Conversation

isaacsas
Copy link
Member

Since indexing problems by a parameter has been dropped by SciMLBase we shouldn't test for this anymore.

@isaacsas isaacsas closed this Dec 30, 2023
@isaacsas isaacsas reopened this Dec 30, 2023
@coveralls
Copy link

coveralls commented Dec 30, 2023

Pull Request Test Coverage Report for Build 7366660627

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.3%) to 89.945%

Totals Coverage Status
Change from base Build 7364429899: 1.3%
Covered Lines: 2299
Relevant Lines: 2556

💛 - Coveralls

@TorkelE
Copy link
Member

TorkelE commented Dec 30, 2023

For reference, not sure whether this is implemented or not, but I think the plan is to make something like;

@parameters k
...
prob.ps[k]

work for parameter indexing. Basically, due to parameters having some property, allowing

prob[k]

directly (like what is done for initial conditions) becomes type unstable. hence the indexing has to be done on the .ps field instead. When it is clear that this is ready, we should move to that instead.

@isaacsas isaacsas closed this Dec 30, 2023
@isaacsas isaacsas reopened this Dec 30, 2023
@ChrisRackauckas
Copy link
Member

For reference, not sure whether this is implemented or not, but I think the plan is to make something like;

Set getp/setp.

But again this is separate from whether these tests should be here. The symbolic indexing interface has all of those things, but we are dropping specifically definition of names by defining f.syms

@isaacsas
Copy link
Member Author

For some reason 1.10 CI seems bugged. Not sure why it is taking over an hour (I've already restarted it once).

@isaacsas
Copy link
Member Author

isaacsas commented Dec 30, 2023

OK, so we should just drop these tests completely then? Since otherwise there is no real way to create a problem with names here (i.e. we don't want to have to use MTK to run the tests...)

@ChrisRackauckas
Copy link
Member

You can define a SymbolCache f.sys

Comment on lines +126 to +130
# since parameters are no longer allowed to be mutated and the preceding will error this
# isn't needed
# if using_params(prob.massaction_jump)
# update_parameters!(prob.massaction_jump, prob.prob.p)
# end
Copy link
Member

Choose a reason for hiding this comment

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

setp will still allow for issues, along with manual mutation.

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably need a custom dispatch on it then to update MassActionJumps?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, @AayushSabharwal can probably help with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be great, thanks!

Copy link
Member

@AayushSabharwal AayushSabharwal Jan 2, 2024

Choose a reason for hiding this comment

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

We now have set_parameter! in SII which is used by setp internally, and can be implemented to do other bookkeeping actions when setp is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AayushSabharwal, can you add the needed dispatches to that when parameters get modified, i.e. something like what we had before when modifying a problem:

if using_params(prob.massaction_jump)
    update_parameters!(prob.massaction_jump, prob.prob.p)
end

Alternatively, @ChrisRackauckas do we want to put this on users (since this could be expensive, so a faster workflow is to only call it once all parameter modifications are finished)? That does leave a gotcha someone could encounter unless they carefully read the docs (which is why the previous behavior here always updated directly after modifying a parameter).

Copy link
Member Author

Choose a reason for hiding this comment

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

But we do put calling the jump update function on users currently when modifying integrators within callbacks, so perhaps it isn't unreasonable to do that.

@isaacsas
Copy link
Member Author

You can define a SymbolCache f.sys

Ok, that is very helpful to know. I was confused from looking at the example test file in SII and thought I’d have to completely build a custom struct satisfying the interface.

@ChrisRackauckas
Copy link
Member

We'll make it a tutorial, https://docs.sciml.ai/SymbolicIndexingInterface/dev/simple_sii_sys/ it's just missing right now.

@AayushSabharwal
Copy link
Member

SciMLBase master supports prob.ps[p] syntax now, so parameter indexing tests can use that instead

@AayushSabharwal
Copy link
Member

AayushSabharwal commented Jan 25, 2024

SciMLBase#master isn't tagged yet, so this won't work

Comment on lines +18 to +21
@test getp(jprob,:p1)(jprob) == 1.0
@test getp(jprob,:p2)(jprob) == 2.0
@test jprob.ps[:p1] == 1.0
@test jprob.ps[:p2] == 2.0
Copy link
Member Author

@isaacsas isaacsas Jan 25, 2024

Choose a reason for hiding this comment

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

Where has the dispatch been added to ensure update_parameters!(jprob.massaction_jump, jprob.prob.p) is called when these parameter changes are made?

@AayushSabharwal
Copy link
Member

bump

@isaacsas
Copy link
Member Author

Handled in #408 now.

@isaacsas isaacsas closed this Mar 20, 2024
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.

5 participants