Skip to content
This repository has been archived by the owner on Nov 14, 2022. It is now read-only.

Add INTER reduce #2

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

RebeccaBecky
Copy link

No description provided.

Copy link
Contributor

@dtasev dtasev left a comment

Choose a reason for hiding this comment

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

Some minor comments for refactoring

I also think it would be useful if the reduce scripts copies the settings.json into the output folder - that way we have a history of all the input data that will make it possible to recreate.

Very interesting case about the usage of reduce_vars.py though, or rather, the no need for it. Maybe we can change it to just have a path to a .json file for now?

That way when rerunning you can point it to a new settings.json file, that maybe you re-configured from the GUI.

Additionally, we can "teach" the webapp to retrieve the json, use the parse_json_settings function and visualise it in a table on the page. This bit just being an ideas for myself really 😄

Comment on lines 3 to 9
sys.path.append('/opt/Mantid/scripts')
sys.path.append('/opt/Mantid/scripts/SANS')
sys.path.append('/opt/Mantid/lib')
sys.path.append('/opt/Mantid/scripts/Inelastic')
sys.path.append('/opt/Mantid/scripts/Engineering')
sys.path.append('/opt/Mantid/scripts/Interface')
sys.path.append('/opt/Mantid/scripts/Diffraction')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need those anymore - they now get appended in the container

Comment on lines 165 to 168
return analysis_mode, first_transmission_run_list, second_transmission_run_list, transmission_processing_instructions, \
processing_instructions, start_overlap, end_overlap, monitor_integration_wavelength_min, \
monitor_integration_wavelength_max, monitor_background_wavelength_min, monitor_background_wavelength_max, wavelength_min, \
wavelength_max, i_zero_monitor_index,detector_correction_type
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be easier to manage if you make a dataclass, something like

from dataclasses import dataclass

@dataclass
class INTERParams:
    analysis_mode
    first_transmission_run_list
    second_transmission_run_list
    transmission_processing_instructions
    processing_instructions
    start_overlap
    end_overlap
    monitor_integration_wavelength_min
    monitor_integration_wavelength_max
    monitor_background_wavelength_min
    monitor_background_wavelength_max
    wavelength_min
    wavelength_max: float
    i_zero_monitor_index
    detector_correction_type

that also allows you to set typed, e.g. wavelength_max: float

Then just make it at the start of the function params = INTERParams() and fill it with data as you go down

Comment on lines 118 to 122
first_transmission_run_list = i[1]
second_transmission_run_list = i[2]
transmission_processing_instructions = i[3]
# Skipping over parameters that are present in the JSON file but not currently used in the reduction
processing_instructions = i[8]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move this into a little helper function, e.g. get_transmission_params that takes i and sets the correct fields. This would make it easier to test as well, because now a unit test can be added for the helper function to make sure it works

min = angle * 0.995
max = angle * 1.005
angle_found = False
for i in rows:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think row is a better name than i, you'll have to rename it in the scope below though

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants