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

Mapping non-source nodes #174

Open
jl-wynen opened this issue Jun 26, 2024 · 6 comments
Open

Mapping non-source nodes #174

jl-wynen opened this issue Jun 26, 2024 · 6 comments

Comments

@jl-wynen
Copy link
Member

It can be useful to map over nodes that have inputs but Sciline / Cyclebane do not allow this. For example, in ESSreflectometry, we want to map over filenames and sample angles. However, the sample angle is by default read from a file. So its provider has inputs and Sciline refuses to map over it.

Here is a simplified example:

import sciline as sl


def foo(i: int) -> float:
    return 10.0 * i


def bar(f: float) -> str:
    return str(f)


pl = sl.Pipeline([foo, bar])
pl = pl.map({float: [2.0, 3.0]})

This fails with ValueError: Mapped node '<class 'float'>' is not a source node.

It can be circumvented using

pl = sl.Pipeline([foo, bar])
pl[float] = 0.0
pl = pl.map({float: [2.0, 3.0]})

but this is not great.

Mapping nodes corresponds to assigning values to them. So it is analogous to setting a parameter value. And since this is allowed for non-source nodes, mapping should also be allowed.

@SimonHeybrock
Copy link
Member

I am not sure this is something that Cyclebane should do: Conceptually, cyclebane.Graph.map adds value attributes to nodes, it does not perform graph manipulation such as deleting edges. I have to think about this. Maybe having this performed in sciline.Pipeline would be (more?) sensible.

@SimonHeybrock
Copy link
Member

Maybe pl[float:].map({float: [...]}) would make sense for this? It is more explicit without hidden magic, but also concise?

@jl-wynen
Copy link
Member Author

How would that work with multiple mapped values? Here is a more complete example of what we would need in reflectometry:

import sciline as sl


def get_sample_angle(filename: str) -> float:
    return 0.1 * len(filename)


def load(filename: str) -> int:
    return len(filename)


def calc(data: int, angle: float) -> tuple[int, float]:
    return data, angle


pl = sl.Pipeline([get_sample_angle, load, calc])
pl = pl.map({str: ['aa', 'bbb'], float: [10.0, 20.0]})
r = sl.compute_mapped(pl, tuple[int, float])

With your suggestion

pl = pl[float:].map({str: ['aa', 'bbb'], float: [10.0, 20.0]})

I get

NotImplementedError: Only single nodes are supported 

@SimonHeybrock
Copy link
Member

NotImplementedError: Only single nodes are supported

Yes, this is not implemented yet, I was a hypothetical syntax.

@SimonHeybrock
Copy link
Member

How would that work with multiple mapped values?

You would need to use __getitem__ once for each of them.

Here is a more complete example of what we would need in reflectometry:

import sciline as sl


def get_sample_angle(filename: str) -> float:
    return 0.1 * len(filename)


def load(filename: str) -> int:
    return len(filename)


def calc(data: int, angle: float) -> tuple[int, float]:
    return data, angle


pl = sl.Pipeline([get_sample_angle, load, calc])
pl = pl.map({str: ['aa', 'bbb'], float: [10.0, 20.0]})
r = sl.compute_mapped(pl, tuple[int, float])

Have you considered other solutions, such as removing the provider, not inserting the provider in the first place, or moving the sample-angle-load into a small helper pipeline that can be added via __setitem__? What is the verdict?

@jl-wynen
Copy link
Member Author

Have you considered other solutions, such as removing the provider, not inserting the provider in the first place, or moving the sample-angle-load into a small helper pipeline that can be added via setitem? What is the verdict?

Not in detail. If in the long run, we want to map over filenames and sample angles in this way, then we would of course adapt the pipeline. But for a quick test, that would be difficult. And this includes removing a parameter after the fact.
This issue is mostly about doing this kind of map with as little code and thought as possible.

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

No branches or pull requests

2 participants