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

modify the requisite on the function complete_state #210

Closed
wants to merge 1 commit into from
Closed

Conversation

awage
Copy link
Contributor

@awage awage commented Jul 19, 2024

Following the discussion in issue #209 I propose to loosen the requisites on the function complete_state for the ProjectedDynamicalSystem. The only important aspect is that the function complete_state should return a valid vector on the original state space.

Unfortunately I don't think we can make this check in the code if we don't know the kind of input y.

@Datseris
Copy link
Member

Datseris commented Jul 19, 2024

I believe we have a classic Y instead of X problem here.

I am not happy with this modification. The projected space is a well defined space. The original space is a well defined space. A projected dynamical system takes states in the projected space and maps them to the projected space. All is well defined. With this PR it isn't any more, because complete_state can take "whatever", and "whatever" is bad practice for designing a clear API that a user can learn simply and intuitively.

Instead of trying to alter this situation, can you tell me what you want to actually achieve? In #209 you only loosely described your situation. Please describe exactly your situation and lets see if this situation can achieve with no changes, or simpler changes that do not change the existing API.

@awage
Copy link
Contributor Author

awage commented Jul 19, 2024

I can give you a simple example:

ds = Systems.lorenz96(5)
projection(u) = [sum(u), sqrt(u[1]^2 + u[2]^2)]
complete_state(y) = [y[1]+2*y[2], y[3]+2*y[4]+y[5]] 
prods = ProjectedDynamicalSystem(ds, projection, complete_state)

The definition will throw an error since the input vector should be of dimension 2.

Let suppose I want to match a vector u from the original state space using this projection. I simply cannot directly. I have to use a workaround somehow.

EDIT: My goal is to map attractors using the original state space using the projected state space, such that

mapper = AttractorsViaRecurrences(prods, grid)
l = mapper(rand(5))

works on the original space of ds.

Maybe a keyword to override the usual behavior will do?

@Datseris
Copy link
Member

Let suppose I want to match a vector u from the original state space using this projection

Why don't you just call y = projection(u)?

mapper = AttractorsViaRecurrences(prods, grid); l = mapper(rand(5))

why don't you do l = mapper(projection(rand(5)) instead?

I don't see why something so simple to resolve at the user level should instead become a convoluted option at the software level. You will need to provide arguments why using the projection at your end is not an option.

@awage
Copy link
Contributor Author

awage commented Jul 19, 2024 via email

@Datseris
Copy link
Member

I know this solution but it does not solve my problem.

From what you have described so far, I don't see how this doesn't solve your problem. Let's say you have a sampler() that samples the original dynamical system. Do you want to call basins_fractions with this sampler, but the mapper should be for the projected system? Then call

projected_sampler = () -> projection(sampler())
fs = basins_fractions(projected_mapper, projected_sampler)

so yeah I will keep an eye out for a lengthier explanation next week!

@awage
Copy link
Contributor Author

awage commented Jul 22, 2024

I close this PR. It breaks the API. I will describe the specifics in the opened issue.

@awage awage closed this Jul 22, 2024
@Datseris Datseris deleted the comp_state branch July 22, 2024 09:11
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.

2 participants