Skip to content

Common proton charge loader#552

Merged
jl-wynen merged 4 commits intomainfrom
load-proton-charge
May 5, 2026
Merged

Common proton charge loader#552
jl-wynen merged 4 commits intomainfrom
load-proton-charge

Conversation

@jl-wynen
Copy link
Copy Markdown
Member

@jl-wynen jl-wynen commented May 1, 2026

This moves the proton charge loader provider from ESSimaging into ESSreduce. I changed the parameters a little to make it fit into the workflow more neatly (ProtonCharge is not a Component and so shouldn't have a NeXusName.)

I did not generalise the normalisation because of #551.

Note that ESSreflectometry calls this ProtonCurrent and has its own providers. Should I rename it and use the same domain type? Or should we wait until we have an actual ESTIA NeXus loader for experimental files?

@jl-wynen jl-wynen added essimaging Issues for essimaging. essreduce Issues for essreduce. labels May 1, 2026
@jl-wynen jl-wynen requested a review from SimonHeybrock May 4, 2026 07:39
Copy link
Copy Markdown
Member

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

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

Note that ESSreflectometry calls this ProtonCurrent and has its own providers. Should I rename it and use the same domain type? Or should we wait until we have an actual ESTIA NeXus loader for experimental files?

If the ProtonCurrent is a charge it should be renamed, if it is actually a current then the workflow should be changed, either to compute current from charge, or use charge directly?



class ProductionInfo(snx.NXsource):
"""A specialized NXsource for neutron production information (accelerator).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is confusing. The accelerator has no neutrons. Production of what?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But the accelerator is used to produce neutrons. I took the name from the nexus files. The group contains

        grp   beam_cycle_id/
        grp   beam_destination/
        grp   beam_destination_mode/
        grp   beam_mode/
        grp   beam_present/
        grp   beam_pulse_end_event_counter/
        grp   beam_pulse_end_event_id/
        grp   beam_pulse_length/
        grp   beam_pulse_start_event_counter/
        grp   beam_state/
        grp   cryo_tt_82025_temperature/
        grp   cryo_tt_82027_temperature/
        grp   cryo_tt_82029_temperature/
        grp   cryo_tt_82031_temperature/
        grp   cryo_tt_82033_temperature/
        grp   cycle_start_event_counter/
        grp   data_buffer_sent_offset/
() 16B  utf-8 depends_on                     .
        grp   flat_top_current/
        grp   fsm_machine_active/
        grp   high_beta_flat_top_current/
        grp   high_beta_pulse_charge/
        grp   high_beta_pulse_width/
        grp   intended_proton_current/
        grp   intended_proton_energy/
() 16B  utf-8 name                           accelerator
        grp   ni_acq_start_event_counter/
        grp   ni_sync_event_counter/
() 16B  utf-8 probe                          proton
        grp   pulse_charge/
        grp   pulse_width/
        grp   raster_pattern/
() 16B  utf-8 target_material                W
        grp   target_segment/
        grp   transformations/
() 16B  utf-8 type                           Spallation Neutron Source

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So you say it corresponds the group name in NeXus? That's a good convention then.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It does approximately, the name is 'neutron_prod_info'.

@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented May 5, 2026

Note that ESSreflectometry calls this ProtonCurrent and has its own providers. Should I rename it and use the same domain type? Or should we wait until we have an actual ESTIA NeXus loader for experimental files?

If the ProtonCurrent is a charge it should be renamed, if it is actually a current then the workflow should be changed, either to compute current from charge, or use charge directly?

ESS NeXus files only encode the charge each second. You could convert that to a current but I doubt that anyone would do that. In reflectometry, the 'proton current' is currently dimensionless. So it is not clear what it represents. @jokasimr can you clarify?

@jokasimr
Copy link
Copy Markdown
Contributor

jokasimr commented May 5, 2026

ESS NeXus files only encode the charge each second. You could convert that to a current but I doubt that anyone would do that. In reflectometry, the 'proton current' is currently dimensionless. So it is not clear what it represents. @jokasimr can you clarify?

  1. It's not correct to say the proton current is dimensionless in reflectometry. The proton current correction in reflectometry is agnostic to the unit of the proton current quantity.

  2. It's true that there are examples in the docs or maybe in the tests where the proton current is assigned a dummy value that is dimensionless.

@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented May 5, 2026

ESS NeXus files only encode the charge each second. You could convert that to a current but I doubt that anyone would do that. In reflectometry, the 'proton current' is currently dimensionless. So it is not clear what it represents. @jokasimr can you clarify?

1. It's not correct to say the `proton current` is dimensionless in reflectometry. The proton current correction in reflectometry is agnostic to the unit of the proton current quantity.

2. It's true that there are examples in the docs or maybe in the tests where the proton current is assigned a dummy value that is dimensionless.

Fine, but what I meant is that I can't tell based on the code what unit it should have. Should it be A or C? If you say it's agnostic, then can we just fix it to C and call it 'proton charge'?

@jokasimr
Copy link
Copy Markdown
Contributor

jokasimr commented May 5, 2026

I don't mind, either "_current" or "_charge" works for me. But it might make sense to use the same convention that ECDC uses when they store the proton current in the nexus files.

@SimonHeybrock
Copy link
Copy Markdown
Member

@jokasimr Can you review the reflectometry-specific changes here? Thanks!

@jl-wynen jl-wynen force-pushed the load-proton-charge branch 3 times, most recently from c944ea7 to cbb6243 Compare May 5, 2026 11:16
@jl-wynen jl-wynen force-pushed the load-proton-charge branch from cbb6243 to 9aa2b0e Compare May 5, 2026 11:36
@jokasimr
Copy link
Copy Markdown
Contributor

jokasimr commented May 5, 2026

Looks good to me 👍

@jl-wynen jl-wynen added this pull request to the merge queue May 5, 2026
Merged via the queue into main with commit c551f7e May 5, 2026
15 checks passed
@jl-wynen jl-wynen deleted the load-proton-charge branch May 5, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

essimaging Issues for essimaging. essreduce Issues for essreduce.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants