Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests for Monitor Normalisation: #2215

Merged
merged 2 commits into from
Jun 7, 2024
Merged

Tests for Monitor Normalisation: #2215

merged 2 commits into from
Jun 7, 2024

Conversation

ashmeigh
Copy link
Collaborator

@ashmeigh ashmeigh commented Jun 4, 2024

Issue

Add test for Monitor Normalisation

Description

This pull of request introduces the load_monitor_log method, which enhances the log file handling process and integrates JSON configuration for monitor normalization settings. This update aims to improve both flexibility and error management.

Testing

Ensure that log files are accurately identified and loaded.
Confirmed the correct application of settings from JSON configurations.
Verified integration does not disrupt existing functionality.

Acceptance Criteria

Run the updated tests to check both log file handling and JSON configuration effects

@coveralls
Copy link

Coverage Status

coverage: 72.735%. remained the same
when pulling 16973ff on mon_norm_test
into 4d2ef3e on main.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Marking as "Request Changes" just to note the things we discussed.

  • Fixes to loading using filename_group.find_log_file()
  • Updating so that this does not remove the Circular Mask from the test_cases

@coveralls
Copy link

Coverage Status

coverage: 72.706%. remained the same
when pulling f482056 on mon_norm_test
into f482056 on main.

@ashmeigh ashmeigh reopened this Jun 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 72.706%. remained the same
when pulling 8d95f52 on mon_norm_test
into f482056 on main.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Looking good now. Just needs the flat/dark lines dropping out of this PR

Comment on lines 147 to 148
elif test_case.pre_run_step == 'add_flats_and_darks':
self.add_flats_and_darks(test_case.params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These 2 lines have sneaked in from another branch.

@coveralls
Copy link

Coverage Status

coverage: 72.75% (+0.04%) from 72.706%
when pulling cfe4dee on mon_norm_test
into f482056 on main.

Copy link
Collaborator

@samtygier-stfc samtygier-stfc left a comment

Choose a reason for hiding this comment

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

Working well now. Code is good.

@samtygier-stfc samtygier-stfc added this pull request to the merge queue Jun 7, 2024
Merged via the queue into main with commit f0def70 Jun 7, 2024
8 checks passed
@samtygier-stfc samtygier-stfc deleted the mon_norm_test branch June 7, 2024 15:42
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.

3 participants