Update for next scipp #461
Conversation
src/scippneutron/instrument_view.py
Outdated
| """ | ||
| if not p3: | ||
| raise _pythreejs_import_error | ||
|
|
||
| import plopp as pp | ||
|
|
||
| positions_var = scipp_obj.meta[positions] | ||
| try: |
There was a problem hiding this comment.
Is the try/except here for backward compatibility, for older versions of Scipp who do not have deprecated_meta? If so, does that mean we don't need to set the minimum scipp requirement to 23.08?
However, there are then places below that make use of deprecated_attrs that are not in a try/except block.
So they would need the version pin. We should either pin min scipp version 23.08 and use deprecated_attrs, or not pin and use try/except. Right now it seems there is a mix of both?
Unless I just completely misunderstood the motivation for the try/except?
There was a problem hiding this comment.
This is more about forward compatibility since it uses coords in the except. So when we fully remove attrs, this code will continue to work.
I was unsure if I should make all uses of attrs and meta backwards compatible. But I decided against it because it would introduce even more noise. So I'd rather go with a lower version limit.
Concerning forward compat, I can update frames.py to fall back to coords. But this makes no sense for load_nexus as that will be removed before attrs.
There was a problem hiding this comment.
I can update frames.py to fall back to coords
👍
src/scippneutron/tof/frames.py
Outdated
| def _meta_or_fallback(x): | ||
| # This can be removed when attrs have been removed in the minimum scipp version. | ||
| try: | ||
| return x.deprectaed_meta |
There was a problem hiding this comment.
| return x.deprectaed_meta | |
| return x.deprecated_meta |
There was a problem hiding this comment.
Should've run the tests locally.
Fixed this now. But found that the mantid and streamaing tests didn't run. Working on it...
|
The tutorials need some work. I may have to rewrite a significant part of 'exploring data' to use data groups. |
|
Now also fixes #462 |
|
I'm waiting until the scipp release before merging. |
|
I changed it to always fall back it |
# Conflicts: # .buildconfig/ci-linux.yml # .buildconfig/ci-macos.yml # .buildconfig/ci-windows.yml # .github/workflows/pr_and_main.yml
Did you mean "not work" or "now work"? |
I meant 'now' |
This won't build yet because it needs
deprecated_attrswith has not been released.