Skip to content

Fix error in image about linspace#560

Closed
nvaytet wants to merge 2 commits intomainfrom
fix-strange-image-linspace-all-error
Closed

Fix error in image about linspace#560
nvaytet wants to merge 2 commits intomainfrom
fix-strange-image-linspace-all-error

Conversation

@nvaytet
Copy link
Copy Markdown
Member

@nvaytet nvaytet commented Apr 29, 2026

We add .value to sc.islinspace.

This was tripping up some builds in Scippneutron: https://github.com/scipp/scippneutron/actions/runs/24816917140/job/72633086173

@nvaytet nvaytet requested a review from jl-wynen April 29, 2026 14:43
@SimonHeybrock
Copy link
Copy Markdown
Member

SimonHeybrock commented Apr 30, 2026

We add .value to sc.islinspace.

This was tripping up some builds in Scippneutron: https://github.com/scipp/scippneutron/actions/runs/24816917140/job/72633086173

The error says:

2026-04-23T04:34:50.8128569Z >       if (canvas.ax.name != 'polar') and all(
2026-04-23T04:34:50.8128837Z             (data.coords[dim].ndim < 2)
2026-04-23T04:34:50.8129196Z             and ((data.coords[dim].dtype == str) or (sc.islinspace(data.coords[dim])))
2026-04-23T04:34:50.8129764Z             for dim in data.dims
2026-04-23T04:34:50.8129977Z         ):
2026-04-23T04:34:50.8130306Z E       scipp._scipp.core.UnitError: The truth value of a variable with unit is undefined.

But why would sc.islinspace return a variable with unit? It should be dtype=bool with no unit (unit=None), shouldn't it?

@nvaytet
Copy link
Copy Markdown
Member Author

nvaytet commented Apr 30, 2026

I don't understand why it's talking about units. I looked a little bit into it, tried to blame to see when it changed.
I didn't find anything conclusive.

On one side, I'm wondering if it's revealing some edge case in scipp or islinspace. On the other side, I'm thinking I can just add the .value and move on, instead of spending some hours digging deep into the issue...

I looked at the versions: before it was failing, it was installing

plopp==24.9.1 scipp==24.8.0

Now it's installing

plopp==26.4.1 scipp==24.8.0

Note that only plopp has changed.

At least it's claiming that:

Because I tried to create an environment with the same versions locally, and I get

mamba env create -n oldscipp python=3.12.* scipp::scipp==24.8.0 plopp==26.4.1

conda-forge/linux-64                                        Using cache
conda-forge/noarch                                          Using cache
scipp/linux-64                                              Using cache
scipp/noarch                                                Using cache

error    libmamba Could not solve for environment specs
    The following packages are incompatible
    ├─ plopp ==26.4.1 * is installable and it requires
    │  └─ scipp >=25.5.0 *, which can be installed;
    └─ scipp ==24.8.0 * is not installable because it conflicts with any installable versions previously reported.
critical libmamba Could not solve for environment specs

Only difference is I am using conda and not uv, but the requirements should work in the same way?

@SimonHeybrock
Copy link
Copy Markdown
Member

Maybe plopp did indeed not use this code branch? Anyway, Claude found that this was indeed a bug and fixed in scipp 24.11.0 by 75308cc66 ("fix: make islinspace unit none", PR scipp#3533). Ths changed the element-op unit lambda from units::one to units::none.

I suppose you should bump the minimum Scipp version instead of applying this?

@nvaytet
Copy link
Copy Markdown
Member Author

nvaytet commented Apr 30, 2026

Should islinspace just return a bool instead of a Variable with unit None? Or is it so that the behaviour is consistent with other similar functions?

I think the changes here don't hurt in any case. But I will bump scipp minimum version in scippneutron.

@nvaytet
Copy link
Copy Markdown
Member Author

nvaytet commented Apr 30, 2026

Only difference is I am using conda and not uv, but the requirements should work in the same way?

I understand now what happens: in Plopp's pyproject, we did not include Scipp as a dependency, only in the plopp[scipp] or plopp[all].
However, in conda, we have just a run_constraint: https://github.com/conda-forge/plopp-feedstock/blob/main/recipe/recipe.yaml#L35

That is out of date with our pyproject.toml: https://github.com/scipp/plopp/blob/main/pyproject.toml#L40

I think we didn't add scipp in the deps, because Scipp also has plopp as an optional runtime dependency, and we thought we would end up in a circular dependency where plopp needs scipp but scipp needs plopp.
However, in practice maybe it just doesn't matter?

Maybe Plopp should just have Scipp as a core dependency? (It's using it internally everywhere)
@MridulS do you have any insights here?

@nvaytet
Copy link
Copy Markdown
Member Author

nvaytet commented Apr 30, 2026

ChatGPT says:

  • make Scipp a hard dependency of Plopp
  • keep Plopp as a soft dependency of Scipp

Then the pip, uv, conda should still happily solve the environments.

  • if you pip install plopp, you would get plopp and scipp (and skip scipp if it is already installed)
  • if you pip install scipp, you get only scipp
  • if you pip install scipp plopp you get both at once, as expected.

@nvaytet
Copy link
Copy Markdown
Member Author

nvaytet commented May 1, 2026

I will close because in the future, if we change islinspace to return a bool instead of a Variable with unit None, then things would break if this is merged.

@nvaytet nvaytet closed this May 1, 2026
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