Skip to content

Add jungfrau internal acquisition plan#1216

Merged
olliesilvester merged 31 commits into
mainfrom
1200_add_jf_internal_acquisition
Sep 26, 2025
Merged

Add jungfrau internal acquisition plan#1216
olliesilvester merged 31 commits into
mainfrom
1200_add_jf_internal_acquisition

Conversation

@olliesilvester

@olliesilvester olliesilvester commented Aug 29, 2025

Copy link
Copy Markdown
Contributor

Part of #1200

Requires DiamondLightSource/dodal#1548

Instructions to reviewer on how to test:

  1. Do thing x
  2. Confirm thing y happens

Checks for reviewer

  • Would the PR title make sense to a user on a set of release notes

@olliesilvester olliesilvester requested a review from a team as a code owner August 29, 2025 15:40
@olliesilvester olliesilvester changed the title 1200 add jf internal acquisition Add jungfrau internal acquisition plan Aug 29, 2025
@olliesilvester olliesilvester changed the base branch from main to 1200_add_basic_jf_plans August 29, 2025 15:41
@olliesilvester olliesilvester force-pushed the 1200_add_jf_internal_acquisition branch from fc765e8 to cb8447e Compare September 1, 2025 15:05
Base automatically changed from 1200_add_basic_jf_plans to main September 17, 2025 14:16
@olliesilvester

Copy link
Copy Markdown
Contributor Author

CI is failing because pilatus was disconnected from zebra in this PR, but there are still plans which require pilatus. Will revert the zebra changes in this PR and do that refactor elsewhere

@codecov

codecov Bot commented Sep 22, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (aee067c) to head (35da083).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1216   +/-   ##
=======================================
  Coverage   90.15%   90.16%           
=======================================
  Files         130      132    +2     
  Lines        7844     7872   +28     
=======================================
+ Hits         7072     7098   +26     
- Misses        772      774    +2     
Components Coverage Δ
i24 SSX 75.27% <ø> (ø)
hyperion 97.79% <ø> (ø)
other 97.94% <96.00%> (-0.06%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, a few comments in the code.

Comment on lines +62 to +63
jungfrau._writer._path_info().filename = file_name # noqa: SLF001
yield from bps.abs_set(jungfrau._writer.file_name, file_name) # noqa: SLF001

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: As I said in https://github.com/DiamondLightSource/dodal/pull/1548/files#r2369303859, I think that a call to a PathProvider will just give a new path. Also, I think that jungfrau._writer.file_name will get overriden by what is in the path provider when open is called anyway. So I think the only line in this function that actually does anything is the first one. Can we have some unit tests on this to confirm what we're actually doing and remove lines that aren't necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, yeah, I must have been rushing this part

"""

if path_of_output_file:
override_file_name(jungfrau, path_of_output_file)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: The name and docs of path_of_output_file implies that it's a full path but override_file_name actually juts wants a file name. Either update override_file_name to take a full path or change do_internal_acquisition to only take a new path filename

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, from doing the tests on the beamline they wanted everything in folders e.g. /dls/i24/data/2025/cm40647-4/jungfrau/2025-09-15/zeo62 contains one set of data and /dls/i24/data/2025/cm40647-4/jungfrau/2025-09-15/zeo63 contains another. So we should let the folder change. Ideally we should also do this increment for them

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if I change override_file_name to allow you to change filename and folder, and by default use the AutoIncrementingPathProvider, then this will increment automatically if a new path hasn't been specified. Does that sound good?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the commissioning JF device now checks against the specified file already existing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if I change override_file_name to allow you to change filename and folder, and by default use the AutoIncrementingPathProvider, then this will increment automatically if a new path hasn't been specified. Does that sound good?

Yes, that's fine. As long as we confirm that's what the AutoIncrementingPathProvider does. I know when we tested other path providers on the beamline we ended up with weird nested date directories

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the AutoIncrementingPathProvider gives us a slightly different format of /dls/i24/data/2025/cm40647-4/jungfrau/2025-09-15/zeo/00000/data_file then /dls/i24/data/2025/cm40647-4/jungfrau/2025-09-15/zeo/00001/data_file. I think this is fine but we should make the beamline aware

@DominicOram

Copy link
Copy Markdown
Contributor

Also, the test looks real, can you take a look?

@olliesilvester olliesilvester force-pushed the 1200_add_jf_internal_acquisition branch from 4603df0 to 824be80 Compare September 24, 2025 09:29
@olliesilvester

Copy link
Copy Markdown
Contributor Author

Tests are passing locally, but will wait for CI to get fixed before re-review

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you. I think just #1216 (comment) then I think the test failure still looks somewhat real (it's on a JF one anyway)

@olliesilvester olliesilvester merged commit de0e584 into main Sep 26, 2025
24 checks passed
@olliesilvester olliesilvester deleted the 1200_add_jf_internal_acquisition branch September 26, 2025 08:57
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