Refactor for next Sciline (Optional, param tables, and more) #135

SimonHeybrock · 2024-04-30T12:31:13Z

Changes:

Add helpers such as set_banks, replacing Pipeline.set_param_table.
Remove complicated multi-step helpers for run and bank merges.
Small changes to work with new way of Optional handling by Sciline.
Change masking.py to work more cleanly without param table with a small local workflow structure change.

Notes:

Depends on Sciline@main, need to update deps once released.
~~Depends on fixes in Cyclebane which are not even in it's main yet.~~

Fixes #97.
Fixes #142.

docs/user-guide/common/beam-center-finder.ipynb

SimonHeybrock · 2024-06-06T06:52:44Z

src/ess/isissans/mantidio.py

@@ -32,10 +32,10 @@ def __getattr__(self, name: str) -> NoReturn:
    # Needed for type annotations
    MatrixWorkspace = object

-Period = NewType('Period', int)
+Period = NewType('Period', int | None)


I was not sure if the Optional should be absorbed into the aliases like this. As it is now it is less explicit on the user side, but requires less changes. If we don't like it we can change later on?

I prefer it like this, never really liked the Optional thing anyway

Outdated.

nvaytet

DId you verify that the results are identical to what you get from main?

nvaytet · 2024-06-06T11:57:20Z

docs/user-guide/isis/zoom.ipynb

    "workflow[DirectBeamFilename] = isis.data.zoom_tutorial_direct_beam()\n",
    "workflow[isis.CalibrationFilename] = isis.data.zoom_tutorial_calibration()\n",
    "workflow[Filename[SampleRun]] = isis.data.zoom_tutorial_sample_run()\n",
    "workflow[Filename[EmptyBeamRun]] = isis.data.zoom_tutorial_empty_beam_run()\n",
    "workflow[isis.SampleOffset] = sc.vector([0.0, 0.0, 0.11], unit='m')\n",
    "workflow[isis.DetectorBankOffset] = sc.vector([0.0, 0.0, 0.5], unit='m')\n",
    "masks = isis.data.zoom_tutorial_mask_filenames()\n",
-    "workflow.set_param_series(PixelMaskFilename, masks)"
+    "workflow = set_pixel_mask_filenames(workflow, masks)"


I wonder if users would find this syntax confusing/unusual.
Everything above seems to be operating in-place on the workflow (using setitem), but this one takes in the workflow as an argument, and returns an updated workflow.

I know we want to try and hide map and reduce from users, so they don't have to do it themselves, but I'm predicting that some will be confused by this.

That said, I'm not sure how to fix it, because you can't set a method on the workflow like workflow.set_pixel_masks_filenames(masks) as it's specific to this workflow...

Maybe it's again a case of leaving it for now and finder a better solution later when we've had more experience using it?

I guess when all this will be hidden inside small widgets, it won't be very important anymore...

Alternatives are:

Make this arguments of the workflow creation, LokiWorkflow(sample_runs=..., ...).

Turn LokiWorkflow into a builder (using builder pattern):
workflow = LokiWorkflow() workflow.set_sample_runs([...]) workflow.build() # or build automatically in `workflow.compute()`, by making this a wrapper.

Turn LokiWorkflow into a wrapper, with internal reference to the pipeline. Dedicated methods can set and update the internal workflow.

I think neither of those options is too crazy, but we should probably give it more thought, and gain some more experience using this.

I opened scipp/essreduce#35 for tracking this.

nvaytet · 2024-06-06T11:59:43Z

docs/user-guide/loki/loki-direct-beam.ipynb

@@ -308,6 +305,7 @@
   "metadata": {},
   "outputs": [],
   "source": [
+    "workflow = sans.set_banks(workflow, banks=['larmor_detector'])\n",


So do we still have to do a mapping when we only have a single bank?

No, I think I put it there having the full Loki in mind, to make it easier to modify in the future. Should I remove it?

I would say yes

nvaytet · 2024-06-06T12:00:14Z

docs/user-guide/loki/loki-direct-beam.ipynb

@@ -558,11 +556,8 @@
    "workflow[DimsToKeep] = []\n",
    "\n",
    "# Set parameter series for file names\n",


Outdated comment?

nvaytet · 2024-06-06T12:03:24Z

src/ess/isissans/mantidio.py

@@ -32,10 +32,10 @@ def __getattr__(self, name: str) -> NoReturn:
    # Needed for type annotations
    MatrixWorkspace = object

-Period = NewType('Period', int)
+Period = NewType('Period', int | None)


I prefer it like this, never really liked the Optional thing anyway

nvaytet · 2024-06-06T12:17:30Z

src/ess/sans/masking.py

+    )
+
+
+def to_detector_mask(


This makes more sense 👍

SimonHeybrock · 2024-06-10T03:47:59Z

DId you verify that the results are identical to what you get from main?

Only by eye, looking at the figures in the notebooks.

nvaytet · 2024-06-11T17:23:58Z

Only by eye, looking at the figures in the notebooks.

I think it would be good to check the values.

SimonHeybrock · 2024-06-12T02:57:04Z

Only by eye, looking at the figures in the notebooks.

I think it would be good to check the values.

I updated the Loki I(Q) test to verify against reference files. I created those by running the same test on main. The files have been added to this repo in this branch. I excluded the "upper-bound" mode for now, since we expect changes there soon (masking?), which would break the test. Since this PR is unrelated to the uncertainty broadcast mode I think we can just add those reference files later, if desired.

nvaytet · 2024-06-15T06:52:44Z

docs/api-reference/index.md

@@ -19,6 +19,10 @@
   :recursive:

   providers
+   set_background_runs


As was suggested in scipp/essdiffraction#63, should we rename to with_*?

Otherwise, everything looks good 👍

SimonHeybrock · 2024-06-17T05:20:20Z

I updated to the released Sciline. I believe all comments have been addressed so I will merge and release once the builds pass.

SimonHeybrock requested a review from nvaytet April 30, 2024 12:31

nvaytet previously approved these changes May 2, 2024

View reviewed changes

docs/user-guide/common/beam-center-finder.ipynb Outdated Show resolved Hide resolved

SimonHeybrock added 6 commits June 3, 2024 08:17

Refactor for Sciline's future handling of Optional

006f345

Remove helper types for run/bank merges from graph

dcc0682

Refactor mask handling

d3ed178

Add make_workflow to perform map/reduce

97e6fbb

Refactor to dedicated functions

23d3d04

Begin fixing earlier changes

f5f54d4

SimonHeybrock force-pushed the sciline-next2 branch from 80cb49b to f5f54d4 Compare June 3, 2024 13:26

SimonHeybrock added 5 commits June 6, 2024 06:01

WIP workflow helpers

9d133d5

Update loki tests

69d7e54

Various fixes

c2701ff

Fix remaining tests

2eaf7e6

Missing params

c1a2a8c

SimonHeybrock changed the title ~~Refactor for Sciline's future handling of Optional~~ Refactor for next Sciline (Optional, param tables, and more) Jun 6, 2024

SimonHeybrock added 2 commits June 6, 2024 08:39

Use fixed cyclebane

cf0786a

Use Scilien from main, for now

59d362b

SimonHeybrock commented Jun 6, 2024

View reviewed changes

SimonHeybrock marked this pull request as ready for review June 6, 2024 06:57

SimonHeybrock requested a review from nvaytet June 6, 2024 06:58

SimonHeybrock assigned nvaytet Jun 6, 2024

Missing

c8b2d14

SimonHeybrock mentioned this pull request Jun 6, 2024

Multiple NeXus files readout scipp/esspolarization#19

Closed

Remove unnecessary detail from notebook

153bd25

nvaytet reviewed Jun 6, 2024

View reviewed changes

jl-wynen mentioned this pull request Jun 7, 2024

Automated regression tests scipp/copier_template#187

Open

Update outdated explanation

aaf864c

SimonHeybrock mentioned this pull request Jun 10, 2024

Pattern for modifying workflows using map and reduce? scipp/essreduce#35

Open

Update deps to get latest cyclebane

9845202

SimonHeybrock added 2 commits June 12, 2024 04:23

Remove example of superfluous set_banks from user guide

a8573c8

Add reference files and verify Loki I(Q)

cd60e0d

nvaytet reviewed Jun 15, 2024

View reviewed changes

SimonHeybrock added 3 commits June 17, 2024 06:57

Rename helpers

21cdda0

Update to released cyclbebane

da6a0bc

Upper bound for NumPy

e83a965

SimonHeybrock force-pushed the sciline-next2 branch from b06272f to e83a965 Compare June 17, 2024 05:15

SimonHeybrock merged commit 7d6a037 into main Jun 17, 2024
3 checks passed

SimonHeybrock deleted the sciline-next2 branch June 17, 2024 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor for next Sciline (Optional, param tables, and more) #135

Refactor for next Sciline (Optional, param tables, and more) #135

SimonHeybrock commented Apr 30, 2024 •

edited

Loading

SimonHeybrock Jun 6, 2024

nvaytet Jun 6, 2024

nvaytet left a comment

nvaytet Jun 6, 2024

nvaytet Jun 6, 2024

SimonHeybrock Jun 10, 2024

nvaytet Jun 6, 2024

SimonHeybrock Jun 10, 2024

nvaytet Jun 11, 2024

nvaytet Jun 6, 2024

nvaytet Jun 6, 2024

nvaytet Jun 6, 2024

SimonHeybrock commented Jun 10, 2024

nvaytet commented Jun 11, 2024

SimonHeybrock commented Jun 12, 2024

nvaytet Jun 15, 2024

SimonHeybrock Jun 17, 2024

SimonHeybrock commented Jun 17, 2024

Refactor for next Sciline (Optional, param tables, and more) #135

Refactor for next Sciline (Optional, param tables, and more) #135

Conversation

SimonHeybrock commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvaytet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock commented Jun 10, 2024

nvaytet commented Jun 11, 2024

SimonHeybrock commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock commented Jun 17, 2024

SimonHeybrock commented Apr 30, 2024 •

edited

Loading